diff options
-rw-r--r-- | src/jit/assertionprop.cpp | 47 | ||||
-rw-r--r-- | src/jit/gentree.h | 2 | ||||
-rw-r--r-- | tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.cs | 49 | ||||
-rw-r--r-- | tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.csproj | 42 | ||||
-rw-r--r-- | tests/src/JIT/Regression/JitBlue/GitHub_6318/app.config | 27 |
5 files changed, 137 insertions, 30 deletions
diff --git a/src/jit/assertionprop.cpp b/src/jit/assertionprop.cpp index 004e7d9903..750e2e30db 100644 --- a/src/jit/assertionprop.cpp +++ b/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/jit/gentree.h b/src/jit/gentree.h index 99ff508bdd..dd02d91464 100644 --- a/src/jit/gentree.h +++ b/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/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.cs b/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.cs new file mode 100644 index 0000000000..af6bfe4015 --- /dev/null +++ b/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<T>.CopyTo + try + { + Foo(Vector<int>.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<int> 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/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.csproj new file mode 100644 index 0000000000..ac9ad0e0df --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_6318/GitHub_6318.csproj @@ -0,0 +1,42 @@ +<?xml version="1.0" encoding="utf-8"?> +<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> + <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" /> + <PropertyGroup> + <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration> + <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform> + <AssemblyName>$(MSBuildProjectName)</AssemblyName> + <SchemaVersion>2.0</SchemaVersion> + <ProjectGuid>{80B4796D-0D4C-46A3-9185-6EEA11DD4090}</ProjectGuid> + <OutputType>Exe</OutputType> + <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids> + <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir> + + <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp> + </PropertyGroup> + <!-- Default configurations to help VS understand the configurations --> + <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "> + </PropertyGroup> + <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "> + </PropertyGroup> + <PropertyGroup> + <DebugType></DebugType> + <Optimize>True</Optimize> + </PropertyGroup> + <ItemGroup> + <Compile Include="$(MSBuildProjectName).cs" /> + </ItemGroup> + <ItemGroup> + <None Include="$(JitPackagesConfigFileDirectory)threading+thread\project.json" /> + <None Include="app.config" /> + </ItemGroup> + <ItemGroup> + <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" /> + </ItemGroup> + <PropertyGroup> + <ProjectJson>$(JitPackagesConfigFileDirectory)threading+thread\project.json</ProjectJson> + <ProjectLockJson>$(JitPackagesConfigFileDirectory)threading+thread\project.lock.json</ProjectLockJson> + </PropertyGroup> + <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" /> + <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "> + </PropertyGroup> +</Project> diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_6318/app.config b/tests/src/JIT/Regression/JitBlue/GitHub_6318/app.config new file mode 100644 index 0000000000..6f7bbd9d2b --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_6318/app.config @@ -0,0 +1,27 @@ +<?xml version="1.0" encoding="utf-8"?> +<configuration> + <runtime> + <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1"> + <dependentAssembly> + <assemblyIdentity name="System.Runtime" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" /> + <bindingRedirect oldVersion="0.0.0.0-4.0.20.0" newVersion="4.0.20.0" /> + </dependentAssembly> + <dependentAssembly> + <assemblyIdentity name="System.Text.Encoding" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" /> + <bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" /> + </dependentAssembly> + <dependentAssembly> + <assemblyIdentity name="System.Threading.Tasks" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" /> + <bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" /> + </dependentAssembly> + <dependentAssembly> + <assemblyIdentity name="System.IO" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" /> + <bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" /> + </dependentAssembly> + <dependentAssembly> + <assemblyIdentity name="System.Reflection" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" /> + <bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" /> + </dependentAssembly> + </assemblyBinding> + </runtime> +</configuration> |