From: Joseph Tremoulet Date: Mon, 18 Jul 2016 16:56:37 +0000 (-0400) Subject: Optimize bounds checks with correct assertion set X-Git-Tag: submit/tizen/20210909.063632~11030^2~9738^2 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=144fd2211a08f0713fca7201b9270066e3f92a77;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Optimize bounds checks with correct assertion set Assertion prop defers removing redundant bounds checks until processing the parent comma, because the rewrite involves the comma as well. The code currently has a bug in that its check for whether the bounds check is redundant also occurs when processing reaches the comma, which is wrong because the assertion set may then include assertions generated by the RHS of the comma (there is code to compensate for the assertion generated by the bounds check itself, which is the LHS of the comma, but no compensation for the RHS). This change moves the analysis of whether bounds checks are redundant to the proper spot in the processing order, and delays only the rewrite to the processing of the comma; the redundancy of the bounds check is communicated via a new opcode-specific flag: GTF_ARR_BOUND_INBND. Fixes dotnet/coreclr#6318. Commit migrated from https://github.com/dotnet/coreclr/commit/16c65845eef41548c5653f14fede5a8c5c0be30f --- diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp index 004e7d9..750e2e3 100644 --- a/src/coreclr/src/jit/assertionprop.cpp +++ b/src/coreclr/src/jit/assertionprop.cpp @@ -3227,25 +3227,13 @@ GenTreePtr Compiler::optAssertionProp_Comma(ASSERT_VALARG_TP assertions const GenTreePtr tree, const GenTreePtr stmt) { - // Process the bounds check as part of the GT_COMMA node since, we need parent pointer to remove nodes. - if (tree->gtGetOp1()->OperGet() == GT_ARR_BOUNDS_CHECK) - { - // Since the GT_COMMA tree gets processed by assertion prop after the child GT_ARR_BOUNDS_CHECK - // node in execution order, bounds check assertions will be included for the parent GT_COMMA node. - // Remove the assertion made by the bounds check tree about itself. Assertion only applies to - // "future" bounds checks. - AssertionIndex index = (AssertionIndex)tree->gtGetOp1()->GetAssertion(); - if (index != NO_ASSERTION_INDEX && optGetAssertion(index)->IsBoundsCheckNoThrow()) - { - BitVecOps::RemoveElemD(apTraits, assertions, index - 1); - GenTreePtr newTree = optAssertionProp_BndsChk(assertions, tree, stmt); - BitVecOps::AddElemD(apTraits, assertions, index - 1); - return newTree; - } - else - { - return optAssertionProp_BndsChk(assertions, tree, stmt); - } + // Remove the bounds check as part of the GT_COMMA node since we need parent pointer to remove nodes. + // When processing visits the bounds check, it sets the throw kind to None if the check is redundant. + if ((tree->gtGetOp1()->OperGet() == GT_ARR_BOUNDS_CHECK) + && ((tree->gtGetOp1()->gtFlags & GTF_ARR_BOUND_INBND) != 0)) + { + optRemoveRangeCheck(tree, stmt, true, GTF_ASG, true /* force remove */); + return optAssertionProp_Update(tree, tree, stmt); } return nullptr; } @@ -3518,7 +3506,7 @@ GenTreePtr Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, const return nullptr; } - assert(tree->gtOper == GT_COMMA && tree->gtGetOp1()->OperGet() == GT_ARR_BOUNDS_CHECK); + assert(tree->gtOper == GT_ARR_BOUNDS_CHECK); BitVecOps::Iter iter(apTraits, assertions); unsigned index = 0; @@ -3533,7 +3521,7 @@ GenTreePtr Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, const continue; } - GenTreeBoundsChk* arrBndsChk = tree->gtGetOp1()->AsBoundsChk(); + GenTreeBoundsChk* arrBndsChk = tree->AsBoundsChk(); // Set 'isRedundant' to true if we can determine that 'arrBndsChk' can be // classified as a redundant bounds check using 'curAssertion' @@ -3609,15 +3597,11 @@ GenTreePtr Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, const gtDispTree(tree, 0, nullptr, true); } #endif - optRemoveRangeCheck(tree, stmt, true, GTF_ASG, true /* force remove */); - GenTreePtr newTree = optAssertionProp_Update(tree, tree, stmt); - if (newTree != nullptr) - { - BitVecOps::RemoveElemD(apTraits, assertions, index - 1); - newTree = optAssertionProp(assertions, tree, stmt); - BitVecOps::AddElemD(apTraits, assertions, index - 1); - return newTree; - } + + // Defer actually removing the tree until processing reaches its parent comma, since + // optRemoveRangeCheck needs to rewrite the whole comma tree. + arrBndsChk->gtFlags |= GTF_ARR_BOUND_INBND; + return nullptr; } return nullptr; } @@ -3703,6 +3687,9 @@ GenTreePtr Compiler::optAssertionProp(ASSERT_VALARG_TP assertions, case GT_NULLCHECK: return optAssertionProp_Ind(assertions, tree, stmt); + case GT_ARR_BOUNDS_CHECK: + return optAssertionProp_BndsChk(assertions, tree, stmt); + case GT_COMMA: return optAssertionProp_Comma(assertions, tree, stmt); diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 99ff508..dd02d91 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -844,6 +844,8 @@ public: #define GTF_NO_OP_NO 0x80000000 // GT_NO_OP --Have the codegenerator generate a special nop + #define GTF_ARR_BOUND_INBND 0x80000000 // GT_ARR_BOUNDS_CHECK -- have proved this check is always in-bounds + //---------------------------------------------------------------- #define GTF_STMT_CMPADD 0x80000000 // GT_STMT -- added by compiler diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.cs new file mode 100644 index 0000000..af6bfe40 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.cs @@ -0,0 +1,49 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. +using System.Runtime.CompilerServices; +using System.Numerics; + +namespace N +{ + public static class C + { + public static int Main(string[] args) + { + // Regression test for an issue with assertion prop leading + // to the wrong exception being thrown from Vector.CopyTo + try + { + Foo(Vector.Zero); + } + catch (System.ArgumentOutOfRangeException) + { + // Caught the right exception + return 100; + } + catch + { + // Caught the wrong exception + return -1; + } + // Caught no exception + return -2; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Foo(Vector vec) + { + int[] a = new int[5]; + // The index [5] is outside the bounds of array 'a', + // so this should throw ArgumentOutOfRangeException. + // There's a subsequent check for whether the destination + // has enough space to receive the vector, which would + // raise an ArgumentException; the bug was that assertion + // prop was using the later exception check to prove the + // prior one "redundant" because the commas confused the + // ordering. + vec.CopyTo(a, 5); + return a[0]; + } + } +} diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.csproj new file mode 100644 index 0000000..ac9ad0e --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.csproj @@ -0,0 +1,42 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {80B4796D-0D4C-46A3-9185-6EEA11DD4090} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + 7a9bfb7d + + + + + + + + + True + + + + + + + + + + + + + $(JitPackagesConfigFileDirectory)threading+thread\project.json + $(JitPackagesConfigFileDirectory)threading+thread\project.lock.json + + + + + diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_6318/app.config b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_6318/app.config new file mode 100644 index 0000000..6f7bbd9 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_6318/app.config @@ -0,0 +1,27 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + +