From 5f4f3d30b7e65cd14d1e3a2939c773e040c7eb22 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 1 Nov 2024 19:37:38 +0100 Subject: [PATCH 1/5] Fix BCE regression --- src/coreclr/jit/rangecheck.cpp | 36 ++++++++++++++++++++++++++++------ src/coreclr/jit/valuenum.h | 11 +++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 1fb068068e845..4a59dfc69f3ce 100644 --- 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); } + ssize_t op2Cns = vnStore->CoercedConstantValue(op2VN); + if (!FitsIn(op2Cns)) + { + return Range(Limit::keUnknown); + } + int icon = -1; if (binop->OperIs(GT_AND)) { // x & cns -> [0..cns] - icon = static_cast(op2->AsIntCon()->IconValue()); + icon = static_cast(op2Cns); } else if (binop->OperIs(GT_UMOD)) { // x % cns -> [0..cns-1] - icon = static_cast(op2->AsIntCon()->IconValue()) - 1; + icon = static_cast(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->IsVNConstantFittingIn(op1->AsOp()->gtGetOp2()->gtVNPair.GetConservative())) { // (x & cns1) >> cns2 -> [0..cns1>>cns2] - int icon1 = static_cast(op1->AsOp()->gtGetOp2()->AsIntCon()->IconValue()); - int icon2 = static_cast(op2->AsIntCon()->IconValue()); + int icon1 = static_cast( + vnStore->CoercedConstantValue(op1->AsOp()->gtGetOp2()->gtVNPair.GetConservative())); + int icon2 = static_cast(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 index 3fa0ae56bc9f5..fe8a1b40ef2f9 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1342,6 +1342,17 @@ class ValueNumStore return ConstantValueInternal(vn DEBUGARG(true)); } + template + bool IsVNConstantFittingIn(ValueNum vn) + { + if (!IsVNConstant(vn) || !varTypeIsIntegral(TypeOfVN(vn))) + { + return false; + } + ssize_t val = CoercedConstantValue(vn); + return FitsIn(val); + } + CORINFO_OBJECT_HANDLE ConstantObjHandle(ValueNum vn) { assert(IsVNObjHandle(vn)); From 1131e42f7489465203b902b2227ff3742d62e86c Mon Sep 17 00:00:00 2001 From: egorbot Date: Sat, 2 Nov 2024 17:50:48 +0100 Subject: [PATCH 2/5] clean up --- src/coreclr/jit/rangecheck.cpp | 8 ++++---- src/coreclr/jit/valuenum.h | 11 +++++++++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 4a59dfc69f3ce..3b8e3d251ea49 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1050,7 +1050,8 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool return Range(Limit::keUnknown); } - int icon = -1; + int op1op2Cns = 0; + int icon = -1; if (binop->OperIs(GT_AND)) { // x & cns -> [0..cns] @@ -1062,11 +1063,10 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool icon = static_cast(op2Cns) - 1; } else if (binop->OperIs(GT_RSH, GT_LSH) && op1->OperIs(GT_AND) && - vnStore->IsVNConstantFittingIn(op1->AsOp()->gtGetOp2()->gtVNPair.GetConservative())) + vnStore->IsVNIntegralConstant(op1->AsOp()->gtGetOp2()->gtVNPair.GetConservative(), &op1op2Cns)) { // (x & cns1) >> cns2 -> [0..cns1>>cns2] - int icon1 = static_cast( - vnStore->CoercedConstantValue(op1->AsOp()->gtGetOp2()->gtVNPair.GetConservative())); + int icon1 = op1op2Cns; int icon2 = static_cast(op2Cns); if ((icon1 >= 0) && (icon2 >= 0) && (icon2 < 32)) { diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index fe8a1b40ef2f9..ec7b6fec04a5b 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1343,14 +1343,21 @@ class ValueNumStore } template - bool IsVNConstantFittingIn(ValueNum vn) + bool IsVNIntegralConstant(ValueNum vn, T* value) { if (!IsVNConstant(vn) || !varTypeIsIntegral(TypeOfVN(vn))) { + *value = 0; return false; } ssize_t val = CoercedConstantValue(vn); - return FitsIn(val); + if (FitsIn(val)) + { + *value = static_cast(val); + return true; + } + *value = 0; + return false; } CORINFO_OBJECT_HANDLE ConstantObjHandle(ValueNum vn) From 455af26093421e79598164910305fb1293c9eb54 Mon Sep 17 00:00:00 2001 From: egorbot Date: Sat, 2 Nov 2024 18:08:20 +0100 Subject: [PATCH 3/5] Fix size regression --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/gentree.cpp | 8 ++++++++ src/coreclr/jit/morph.cpp | 8 ++++---- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 56410dfd3035a..df7e9bcd80ce1 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3018,6 +3018,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 index 62cbaedf55aaa..c01cd6575b7a4 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7618,6 +7618,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(fieldOffset), fieldSeq); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index d3e35988b13e1..c2a73426767c6 100644 --- 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(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); From 6b2633559799358496c9d1bba070cc9545fc857e Mon Sep 17 00:00:00 2001 From: egorbot Date: Sat, 2 Nov 2024 18:26:08 +0100 Subject: [PATCH 4/5] add a test --- .../JitBlue/Runtime_109365/Runtime_109365.cs | 28 +++++++++++++++++++ .../Runtime_109365/Runtime_109365.csproj | 8 ++++++ 2 files changed, 36 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs b/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs new file mode 100644 index 0000000000000..a878e7adf88cd --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs @@ -0,0 +1,28 @@ +// 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 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 new file mode 100644 index 0000000000000..de6d5e08882e8 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.csproj @@ -0,0 +1,8 @@ + + + True + + + + + From 6319867fbb08b33d815761659f6c0e4fa54ce100 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sat, 2 Nov 2024 22:01:59 +0100 Subject: [PATCH 5/5] Update Runtime_109365.cs --- .../JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs b/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs index a878e7adf88cd..27829167a7d8e 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_109365/Runtime_109365.cs @@ -3,6 +3,7 @@ using System; using System.Runtime.CompilerServices; +using System.Threading; using Xunit; public class Runtime_109365