Exception sets: debug checker & fixes (#63539)

* Add a simple exception sets checker * Add asserts to catch missing nodes * Fix normal VN printing * Fix JTRUE VNs * Fix PHI VNs * Update VNs for "this" ARGPLACE node * Tolerate missing VNs on PHI_ARGs We do not update them after numbering the loops. (Though perhaps we should) * Tolerate unreachable blocks * Fix exception sets for VNF_PtrTo VNFuncs * Add VNUniqueWithExc * Add VNPUniqueWithExc * Fix arrays * Consistently give location nodes VNForVoid And always add exception sets for them. This will simplify the exception set propagation code for assignments. * Fix CSE * Fix GT_RETURN * Fix LCLHEAP * Fix GT_ARR_ELEM * Fix unique HWI * Fix unique SIMD * Fix GT_SWITCH * Fix CKFINITE * Fix HWI loads * Fix fgValueNumberAddExceptionSetForIndirection The method does not need to add the exception set for the base address. Additionally, the way it did add the sets, by unioning with normal value numbers, lost all exceptions not coming from the base address. This was fine for the unary loads, but broke the HWI loads that could have exceptions coming from not just the address. * Fix GT_RETFILT * Fix INIT_VAL * Fix DYN_BLK * Fix FIELD_LIST * De-pessimize CkFinite * Add a test for HWIs * Add a test for LCLHEAP
dotnet · Jan 20, 2022 · 9bc4439 · 9bc4439
1 parent 1fc2352
commit 9bc4439
Show file tree

Hide file tree

Showing 8 changed files with 500 additions and 214 deletions.
diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
@@ -5432,7 +5432,7 @@ class Compiler
 
     // Requires that "tree" is a GT_IND marked as an array index, and that its address argument
     // has been parsed to yield the other input arguments.  If evaluation of the address
-    // can raise exceptions, those should be captured in the exception set "excVN."
+    // can raise exceptions, those should be captured in the exception set "addrXvnp".
     // Assumes that "elemTypeEq" is the (equivalence class rep) of the array element type.
     // Marks "tree" with the VN for H[elemTypeEq][arrVN][inx][fldSeq] (for the liberal VN; a new unique
     // VN for the conservative VN.)  Also marks the tree's argument as the address of an array element.
@@ -5443,14 +5443,14 @@ class Compiler
                                       CORINFO_CLASS_HANDLE elemTypeEq,
                                       ValueNum             arrVN,
                                       ValueNum             inxVN,
-                                      ValueNum             excVN,
+                                      ValueNumPair         addrXvnp,
                                       FieldSeqNode*        fldSeq);
 
-    // Requires "funcApp" to be a VNF_PtrToArrElem, and "addrXvn" to represent the exception set thrown
+    // Requires "funcApp" to be a VNF_PtrToArrElem, and "addrXvnp" to represent the exception set thrown
     // by evaluating the array index expression "tree".  Returns the value number resulting from
     // dereferencing the array in the current GcHeap state.  If "tree" is non-null, it must be the
     // "GT_IND" that does the dereference, and it is given the returned value number.
-    ValueNum fgValueNumberArrIndexVal(GenTree* tree, struct VNFuncApp* funcApp, ValueNum addrXvn);
+    ValueNum fgValueNumberArrIndexVal(GenTree* tree, VNFuncApp* funcApp, ValueNumPair addrXvnp);
 
     // Compute the value number for a byref-exposed load of the given type via the given pointerVN.
     ValueNum fgValueNumberByrefExposedLoad(var_types type, ValueNum pointerVN);
@@ -5560,6 +5560,10 @@ class Compiler
     // Adds the exception sets for the current tree node
     void fgValueNumberAddExceptionSet(GenTree* tree);
 
+#ifdef DEBUG
+    void fgDebugCheckExceptionSets();
+#endif
+
     // These are the current value number for the memory implicit variables while
     // doing value numbering.  These are the value numbers under the "liberal" interpretation
     // of memory values; the "conservative" interpretation needs no VN, since every access of

diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp
@@ -829,7 +829,8 @@ bool Compiler::optValnumCSE_Locate()
                     continue;
                 }
 
-                if (ValueNumStore::isReservedVN(tree->GetVN(VNK_Liberal)))
+                ValueNum valueVN = vnStore->VNNormalValue(tree->GetVN(VNK_Liberal));
+                if (ValueNumStore::isReservedVN(valueVN) && (valueVN != ValueNumStore::VNForNull()))
                 {
                     continue;
                 }

diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp
diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h
@@ -494,6 +494,8 @@ class ValueNumStore
     // Both arguments must be either VNForEmptyExcSet() or applications of VNF_ExcSetCons.
     bool VNExcIsSubset(ValueNum vnFullSet, ValueNum vnCandidateSet);
 
+    bool VNPExcIsSubset(ValueNumPair vnpFullSet, ValueNumPair vnpCandidateSet);
+
     // Returns "true" iff "vn" is an application of "VNF_ValWithExc".
     bool VNHasExc(ValueNum vn)
     {
@@ -528,6 +530,12 @@ class ValueNumStore
     // Keeps any Exception set values
     ValueNumPair VNPMakeNormalUniquePair(ValueNumPair vnp);
 
+    // A new unique value with the given exception set.
+    ValueNum VNUniqueWithExc(var_types type, ValueNum vnExcSet);
+
+    // A new unique VN pair with the given exception set pair.
+    ValueNumPair VNPUniqueWithExc(var_types type, ValueNumPair vnpExcSet);
+
     // If "vn" is a "VNF_ValWithExc(norm, excSet)" value, returns the "norm" argument; otherwise,
     // just returns "vn".
     // The Normal value is the value number of the expression when no exceptions occurred

diff --git a/src/tests/JIT/opt/ValueNumbering/ExceptionSetsPropagation_Hwi.cs b/src/tests/JIT/opt/ValueNumbering/ExceptionSetsPropagation_Hwi.cs
@@ -0,0 +1,40 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.Intrinsics;
+using System.Runtime.CompilerServices;
+
+// We're testing whether HWI nodes with > 2 operands propagate exception sets correctly.
+//
+class ExceptionSetsPropagation_Hwi
+{
+    public static int Main()
+    {
+        try
+        {
+            Problem(-1, false, false);
+        }
+        catch (OverflowException)
+        {
+            return 100;
+        }
+
+        return 101;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static bool Problem(int a, bool c1, bool c2)
+    {
+        var zero = 0;
+        c1 = a == 0;
+        c2 = c1;
+
+        if ((Vector128.Create((int)checked((uint)a), a, a, a).GetElement(0) * zero) + a == 0)
+        {
+            return false;
+        }
+
+        return c2;
+    }
+}
diff --git a/src/tests/JIT/opt/ValueNumbering/ExceptionSetsPropagation_Hwi.csproj b/src/tests/JIT/opt/ValueNumbering/ExceptionSetsPropagation_Hwi.csproj
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/tests/JIT/opt/ValueNumbering/ExceptionSetsPropagation_LclHeap.il b/src/tests/JIT/opt/ValueNumbering/ExceptionSetsPropagation_LclHeap.il
@@ -0,0 +1,75 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+.assembly extern xunit.core { }
+.assembly extern System.Runtime { }
+.assembly extern System.Console { }
+
+.assembly ExceptionSetsPropagation_LclHeap { }
+
+.class ExceptionSetsPropagation_LclHeap extends [System.Runtime]System.Object
+{
+    .method public static int32 Main()
+    {
+        .custom instance void [xunit.core]Xunit.FactAttribute::.ctor() = (01 00 00 00)
+        .entrypoint
+
+        .try
+        {
+            ldc.i4 300
+            conv.i
+            call bool ExceptionSetsPropagation_LclHeap::Problem(native int)
+            pop
+            leave FAIL
+        }
+        catch [System.Runtime]System.OverflowException
+        {
+            pop
+            leave SUCCESS
+        }
+
+    SUCCESS:
+        ldc.i4 100
+        ret
+
+    FAIL:
+        ldc.i4 101
+        ret
+    }
+
+    .method private static bool Problem(native int a)
+    {
+        .locals (bool c1, bool c2)
+
+        ldarg a
+        localloc
+        ldc.i4 0
+        conv.i
+        and
+        ldarg a
+        ceq
+        stloc c1
+
+        ldloc c1
+        stloc c2
+
+        ldarg a
+        conv.ovf.i1
+        conv.i
+        localloc
+        ldc.i4 0
+        conv.i
+        and
+        ldarg a
+        ceq
+        brtrue RET
+
+        ldstr "The expected OverflowException was not thrown!"
+        call void [System.Console]System.Console::WriteLine(string)
+
+    RET:
+        ldloc c2
+        ret
+    }
+}
+
diff --git a/src/tests/JIT/opt/ValueNumbering/ExceptionSetsPropagation_LclHeap.ilproj b/src/tests/JIT/opt/ValueNumbering/ExceptionSetsPropagation_LclHeap.ilproj
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk.IL">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).il" />
+  </ItemGroup>
+</Project>