JIT: Fix BCE regression (#109466)

dotnet · Nov 20, 2024 · d0b4ca6 · d0b4ca6
1 parent db6cef1
commit d0b4ca6
Show file tree

Hide file tree

Showing 7 changed files with 99 additions and 11 deletions.
diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
@@ -3069,6 +3069,7 @@ class Compiler
                                 GenTree*   op2  = nullptr);
 
     GenTreeIntCon* gtNewIconNode(ssize_t value, var_types type = TYP_INT);
+    GenTreeIntCon* gtNewIconNodeWithVN(Compiler* comp, ssize_t value, var_types type = TYP_INT);
     GenTreeIntCon* gtNewIconNode(unsigned fieldOffset, FieldSeq* fieldSeq);
     GenTreeIntCon* gtNewNull();
     GenTreeIntCon* gtNewTrue();

diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp
@@ -7630,6 +7630,14 @@ GenTreeIntCon* Compiler::gtNewIconNode(ssize_t value, var_types type)
     return new (this, GT_CNS_INT) GenTreeIntCon(type, value);
 }
 
+GenTreeIntCon* Compiler::gtNewIconNodeWithVN(Compiler* comp, ssize_t value, var_types type)
+{
+    assert(genActualType(type) == type);
+    GenTreeIntCon* cns = new (this, GT_CNS_INT) GenTreeIntCon(type, value);
+    comp->fgUpdateConstTreeValueNumber(cns);
+    return cns;
+}
+
 GenTreeIntCon* Compiler::gtNewIconNode(unsigned fieldOffset, FieldSeq* fieldSeq)
 {
     return new (this, GT_CNS_INT) GenTreeIntCon(TYP_I_IMPL, static_cast<ssize_t>(fieldOffset), fieldSeq);

diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp
@@ -8699,8 +8699,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
                     if ((mulOrDiv->OperIs(GT_DIV) && (constVal != -1) && (constVal != 1)) ||
                         (mulOrDiv->OperIs(GT_MUL) && !mulOrDiv->gtOverflow()))
                     {
-                        GenTree* newOp1 = op1op1;                                      // a
-                        GenTree* newOp2 = gtNewIconNode(-constVal, op1op2->TypeGet()); // -C
+                        GenTree* newOp1 = op1op1;                                                  // a
+                        GenTree* newOp2 = gtNewIconNodeWithVN(this, -constVal, op1op2->TypeGet()); // -C
                         mulOrDiv->gtOp1 = newOp1;
                         mulOrDiv->gtOp2 = newOp2;
                         mulOrDiv->SetVNsFromNode(tree);
@@ -10735,7 +10735,7 @@ GenTree* Compiler::fgOptimizeMultiply(GenTreeOp* mul)
                 }
 
                 // change the multiplication into a smaller multiplication (by 3, 5 or 9) and a shift
-                op1        = gtNewOperNode(GT_MUL, mul->TypeGet(), op1, gtNewIconNode(factor, mul->TypeGet()));
+                op1 = gtNewOperNode(GT_MUL, mul->TypeGet(), op1, gtNewIconNodeWithVN(this, factor, mul->TypeGet()));
                 mul->gtOp1 = op1;
                 fgMorphTreeDone(op1);
 
@@ -11731,7 +11731,7 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree)
     const var_types type = tree->TypeGet();
 
     const size_t   cnsValue = (static_cast<size_t>(tree->gtOp2->AsIntConCommon()->IntegralValue())) - 1;
-    GenTree* const newTree  = gtNewOperNode(GT_AND, type, tree->gtOp1, gtNewIconNode(cnsValue, type));
+    GenTree* const newTree  = gtNewOperNode(GT_AND, type, tree->gtOp1, gtNewIconNodeWithVN(this, cnsValue, type));
 
     INDEBUG(newTree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
 

diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp
@@ -1019,31 +1019,55 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool
     GenTree* op1 = binop->gtGetOp1();
     GenTree* op2 = binop->gtGetOp2();
 
+    ValueNum op1VN = op1->gtVNPair.GetConservative();
+    ValueNum op2VN = op2->gtVNPair.GetConservative();
+
+    ValueNumStore* vnStore = m_pCompiler->vnStore;
+
+    bool op1IsCns = vnStore->IsVNConstant(op1VN);
+    bool op2IsCns = vnStore->IsVNConstant(op2VN);
+
+    if (binop->OperIsCommutative() && op1IsCns && !op2IsCns)
+    {
+        // Normalize constants to the right for commutative operators.
+        std::swap(op1, op2);
+        std::swap(op1VN, op2VN);
+        std::swap(op1IsCns, op2IsCns);
+    }
+
     // Special cases for binops where op2 is a constant
     if (binop->OperIs(GT_AND, GT_RSH, GT_LSH, GT_UMOD))
     {
-        if (!op2->IsIntCnsFitsInI32())
+        if (!op2IsCns)
         {
             // only cns is supported for op2 at the moment for &,%,<<,>> operators
             return Range(Limit::keUnknown);
         }
 
-        int icon = -1;
+        ssize_t op2Cns = vnStore->CoercedConstantValue<ssize_t>(op2VN);
+        if (!FitsIn<int>(op2Cns))
+        {
+            return Range(Limit::keUnknown);
+        }
+
+        int op1op2Cns = 0;
+        int icon      = -1;
         if (binop->OperIs(GT_AND))
         {
             // x & cns -> [0..cns]
-            icon = static_cast<int>(op2->AsIntCon()->IconValue());
+            icon = static_cast<int>(op2Cns);
         }
         else if (binop->OperIs(GT_UMOD))
         {
             // x % cns -> [0..cns-1]
-            icon = static_cast<int>(op2->AsIntCon()->IconValue()) - 1;
+            icon = static_cast<int>(op2Cns) - 1;
         }
-        else if (binop->OperIs(GT_RSH, GT_LSH) && op1->OperIs(GT_AND) && op1->AsOp()->gtGetOp2()->IsIntCnsFitsInI32())
+        else if (binop->OperIs(GT_RSH, GT_LSH) && op1->OperIs(GT_AND) &&
+                 vnStore->IsVNIntegralConstant<int>(op1->AsOp()->gtGetOp2()->gtVNPair.GetConservative(), &op1op2Cns))
         {
             // (x & cns1) >> cns2 -> [0..cns1>>cns2]
-            int icon1 = static_cast<int>(op1->AsOp()->gtGetOp2()->AsIntCon()->IconValue());
-            int icon2 = static_cast<int>(op2->AsIntCon()->IconValue());
+            int icon1 = op1op2Cns;
+            int icon2 = static_cast<int>(op2Cns);
             if ((icon1 >= 0) && (icon2 >= 0) && (icon2 < 32))
             {
                 icon = binop->OperIs(GT_RSH) ? (icon1 >> icon2) : (icon1 << icon2);

diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h
@@ -1342,6 +1342,24 @@ class ValueNumStore
         return ConstantValueInternal<T>(vn DEBUGARG(true));
     }
 
+    template <typename T>
+    bool IsVNIntegralConstant(ValueNum vn, T* value)
+    {
+        if (!IsVNConstant(vn) || !varTypeIsIntegral(TypeOfVN(vn)))
+        {
+            *value = 0;
+            return false;
+        }
+        ssize_t val = CoercedConstantValue<ssize_t>(vn);
+        if (FitsIn<T>(val))
+        {
+            *value = static_cast<T>(val);
+            return true;
+        }
+        *value = 0;
+        return false;
+    }
+
     CORINFO_OBJECT_HANDLE ConstantObjHandle(ValueNum vn)
     {
         assert(IsVNObjHandle(vn));

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs b/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs
@@ -0,0 +1,29 @@
+// 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.CompilerServices;
+using System.Threading;
+using Xunit;
+
+public class Runtime_109365
+{
+    [Fact]
+    public static void TestEntryPoint()
+    {
+        var c = new Runtime_109365();
+        for (var i = 0; i < 1000; i++) // triggers tier-1
+        {
+            c.Hash(i);
+            Thread.Sleep(1);
+        }
+    }
+
+    private readonly static int[] _perm = [1,2,3,4];
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public int Hash(int value)
+    {
+        return _perm[value & (_perm.Length - 1)];
+    }
+}
diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.csproj
@@ -0,0 +1,8 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>