From f7e997ae1d4311c0029d69fc542ce1aacb48a72a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 22 Mar 2025 15:57:17 +0100 Subject: [PATCH 1/7] Remove bounds checks for Log2 function --- src/coreclr/jit/rangecheck.cpp | 77 ++++++++++++++++++- src/coreclr/jit/valuenum.cpp | 6 ++ .../Text/FormattingHelpers.CountDigits.cs | 6 +- 3 files changed, 83 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 7d7e20fde9daff..bd04c2c9cc2b71 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1044,10 +1044,77 @@ void RangeCheck::MergeAssertion(BasicBlock* block, GenTree* op, Range* pRange DE } } +//------------------------------------------------------------------------ +// IsLog2: Determine if the value number is a log2 pattern, which is +// XOR(LZCNT32(OR(x, 1), 31) or XOR(LZCNT64(OR(x, 1), 63)). +// This represents what BitOperations.Log2/int.Log2/long.Log2 would do. +// +// Arguments: +// comp - the compiler instance +// vn - the value number to analyze +// upperBound - if not null, will be set to the upper bound of the log2 pattern (31 or 63) +// +// Return value: +// true if the value number is a log2 pattern, false otherwise. +// +static bool IsLog2(Compiler* comp, ValueNum vn, int* upperBound = nullptr) +{ + VNFuncApp funcApp; + + // First, look for "X ^ 31" or "X ^ 63" patterns... + int xorBy; + if (comp->vnStore->GetVNFunc(vn, &funcApp) && (funcApp.m_func == VNF_XOR) && + comp->vnStore->IsVNIntegralConstant(funcApp.m_args[1], &xorBy) && ((xorBy == 31) || (xorBy == 63))) + { + // ...where that X has to be either LZCNT32 (in case of ^ 31) or LZCNT64 (in case of ^ 63) + + // Drop any integer cast if any, we're dealing with [0..63] range, any integer cast is redundant. + ValueNum lzcntVN = funcApp.m_args[0]; + VNFuncApp castFuncApp; + if (comp->vnStore->GetVNFunc(lzcntVN, &castFuncApp) && (castFuncApp.m_func == VNF_Cast) && + varTypeIsIntegral(comp->vnStore->TypeOfVN(lzcntVN))) + { + lzcntVN = castFuncApp.m_args[0]; + } + + VNFunc lzcnFunc = (xorBy == 31) ? VNF_HWI_LZCNT_LeadingZeroCount : VNF_HWI_LZCNT_X64_LeadingZeroCount; + VNFuncApp lzcntFuncApp; + if (comp->vnStore->GetVNFunc(lzcntVN, &lzcntFuncApp) && (lzcntFuncApp.m_func == lzcnFunc)) + { + // Last, check if the argument of the LZCNT32/LZCNT64 is "OR(..., 1)". + int orBy; + VNFuncApp orFuncApp; + if (comp->vnStore->GetVNFunc(lzcntFuncApp.m_args[0], &orFuncApp) && (orFuncApp.m_func == VNF_OR) && + comp->vnStore->IsVNIntegralConstant(orFuncApp.m_args[1], &orBy) && orBy == 1) + { + if (upperBound != nullptr) + { + *upperBound = xorBy; + } + return true; + } + } + } + return false; +} + // Compute the range for a binary operation. Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool monIncreasing DEBUGARG(int indent)) { - assert(binop->OperIs(GT_ADD, GT_AND, GT_RSH, GT_RSZ, GT_LSH, GT_UMOD, GT_MUL)); + assert(binop->OperIs(GT_ADD, GT_XOR, GT_AND, GT_RSH, GT_RSZ, GT_LSH, GT_UMOD, GT_MUL)); + + // For XOR we only care about Log2 pattern for now + if (binop->OperIs(GT_XOR)) + { + ValueNum xorVN = m_pCompiler->vnStore->VNConservativeNormalValue(binop->gtVNPair); + int upperBound; + if (IsLog2(m_pCompiler, xorVN, &upperBound)) + { + assert(upperBound > 0); + return Range(Limit(Limit::keConstant, 0), Limit(Limit::keConstant, upperBound)); + } + return Range(Limit(Limit::keUnknown)); + } GenTree* op1 = binop->gtGetOp1(); GenTree* op2 = binop->gtGetOp2(); @@ -1509,6 +1576,12 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Ran { overflows = false; } + else if (expr->OperIs(GT_XOR) && + IsLog2(m_pCompiler, m_pCompiler->vnStore->VNConservativeNormalValue(expr->gtVNPair))) + { + // For XOR we only care about Log2 pattern for now, which never overflows. + overflows = false; + } // Walk through phi arguments to check if phi arguments involve arithmetic that overflows. else if (expr->OperIs(GT_PHI)) { @@ -1596,7 +1669,7 @@ Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monIncreas MergeAssertion(block, expr, &range DEBUGARG(indent + 1)); } // compute the range for binary operation - else if (expr->OperIs(GT_ADD, GT_AND, GT_RSH, GT_RSZ, GT_LSH, GT_UMOD, GT_MUL)) + else if (expr->OperIs(GT_XOR, GT_ADD, GT_AND, GT_RSH, GT_RSZ, GT_LSH, GT_UMOD, GT_MUL)) { range = ComputeRangeForBinOp(block, expr->AsOp(), monIncreasing DEBUGARG(indent + 1)); } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 9e223650129306..93984f181e76b7 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2766,6 +2766,12 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V { std::swap(arg0VN, arg1VN); } + + // Try to keep constants on the right side. + if (IsVNConstant(arg0VN) && !IsVNConstant(arg1VN)) + { + std::swap(arg0VN, arg1VN); + } } // Have we already assigned a ValueNum for 'func'('arg0VN','arg1VN') ? diff --git a/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/FormattingHelpers.CountDigits.cs b/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/FormattingHelpers.CountDigits.cs index 35f7bba785ae55..5c507bff22f760 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/FormattingHelpers.CountDigits.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/FormattingHelpers.CountDigits.cs @@ -24,8 +24,7 @@ public static int CountDigits(ulong value) ]; Debug.Assert(log2ToPow10.Length == 64); - // TODO: Replace with log2ToPow10[BitOperations.Log2(value)] once https://github.com/dotnet/runtime/issues/79257 is fixed - nint elementOffset = Unsafe.Add(ref MemoryMarshal.GetReference(log2ToPow10), BitOperations.Log2(value)); + nint elementOffset = log2ToPow10[(int)ulong.Log2(value)]; // Read the associated power of 10. ReadOnlySpan powersOf10 = @@ -102,8 +101,7 @@ public static int CountDigits(uint value) ]; Debug.Assert(table.Length == 32, "Every result of uint.Log2(value) needs a long entry in the table."); - // TODO: Replace with table[uint.Log2(value)] once https://github.com/dotnet/runtime/issues/79257 is fixed - long tableValue = Unsafe.Add(ref MemoryMarshal.GetReference(table), uint.Log2(value)); + long tableValue = table[(int)uint.Log2(value)]; return (int)((value + tableValue) >> 32); } From 6132b52d90dadaaeb93599a0be7f7979f16abf76 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 22 Mar 2025 16:30:07 +0100 Subject: [PATCH 2/7] clean up --- src/coreclr/jit/rangecheck.cpp | 41 +++++++++++++--------------------- src/coreclr/jit/valuenum.cpp | 31 +++++++++++++++++++++++++ src/coreclr/jit/valuenum.h | 3 +++ 3 files changed, 49 insertions(+), 26 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index bd04c2c9cc2b71..3b1a28e25e65f8 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1050,42 +1050,32 @@ void RangeCheck::MergeAssertion(BasicBlock* block, GenTree* op, Range* pRange DE // This represents what BitOperations.Log2/int.Log2/long.Log2 would do. // // Arguments: -// comp - the compiler instance -// vn - the value number to analyze +// vnStore - the value number store +// vn - the value number to analyze // upperBound - if not null, will be set to the upper bound of the log2 pattern (31 or 63) // // Return value: // true if the value number is a log2 pattern, false otherwise. // -static bool IsLog2(Compiler* comp, ValueNum vn, int* upperBound = nullptr) +static bool IsLog2(ValueNumStore* vnStore, ValueNum vn, int* upperBound = nullptr) { - VNFuncApp funcApp; - // First, look for "X ^ 31" or "X ^ 63" patterns... - int xorBy; - if (comp->vnStore->GetVNFunc(vn, &funcApp) && (funcApp.m_func == VNF_XOR) && - comp->vnStore->IsVNIntegralConstant(funcApp.m_args[1], &xorBy) && ((xorBy == 31) || (xorBy == 63))) + int xorBy; + ValueNum op1, op2; + if (vnStore->IsBinFunc(vn, VNF_XOR, &op1, &op2) && vnStore->IsVNIntegralConstant(op2, &xorBy) && + ((xorBy == 31) || (xorBy == 63))) { // ...where that X has to be either LZCNT32 (in case of ^ 31) or LZCNT64 (in case of ^ 63) // Drop any integer cast if any, we're dealing with [0..63] range, any integer cast is redundant. - ValueNum lzcntVN = funcApp.m_args[0]; - VNFuncApp castFuncApp; - if (comp->vnStore->GetVNFunc(lzcntVN, &castFuncApp) && (castFuncApp.m_func == VNF_Cast) && - varTypeIsIntegral(comp->vnStore->TypeOfVN(lzcntVN))) - { - lzcntVN = castFuncApp.m_args[0]; - } + vnStore->IsBinFunc(op1, VNF_Cast, &op1); - VNFunc lzcnFunc = (xorBy == 31) ? VNF_HWI_LZCNT_LeadingZeroCount : VNF_HWI_LZCNT_X64_LeadingZeroCount; - VNFuncApp lzcntFuncApp; - if (comp->vnStore->GetVNFunc(lzcntVN, &lzcntFuncApp) && (lzcntFuncApp.m_func == lzcnFunc)) + VNFunc lzcnFunc = (xorBy == 31) ? VNF_HWI_LZCNT_LeadingZeroCount : VNF_HWI_LZCNT_X64_LeadingZeroCount; + if (vnStore->IsBinFunc(op1, lzcnFunc, &op1)) { // Last, check if the argument of the LZCNT32/LZCNT64 is "OR(..., 1)". - int orBy; - VNFuncApp orFuncApp; - if (comp->vnStore->GetVNFunc(lzcntFuncApp.m_args[0], &orFuncApp) && (orFuncApp.m_func == VNF_OR) && - comp->vnStore->IsVNIntegralConstant(orFuncApp.m_args[1], &orBy) && orBy == 1) + int orBy; + if (vnStore->IsBinFunc(op1, VNF_OR, &op1, &op2) && vnStore->IsVNIntegralConstant(op2, &orBy) && (orBy == 1)) { if (upperBound != nullptr) { @@ -1106,9 +1096,8 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool // For XOR we only care about Log2 pattern for now if (binop->OperIs(GT_XOR)) { - ValueNum xorVN = m_pCompiler->vnStore->VNConservativeNormalValue(binop->gtVNPair); - int upperBound; - if (IsLog2(m_pCompiler, xorVN, &upperBound)) + int upperBound; + if (IsLog2(m_pCompiler->vnStore, m_pCompiler->vnStore->VNConservativeNormalValue(binop->gtVNPair), &upperBound)) { assert(upperBound > 0); return Range(Limit(Limit::keConstant, 0), Limit(Limit::keConstant, upperBound)); @@ -1577,7 +1566,7 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Ran overflows = false; } else if (expr->OperIs(GT_XOR) && - IsLog2(m_pCompiler, m_pCompiler->vnStore->VNConservativeNormalValue(expr->gtVNPair))) + IsLog2(m_pCompiler->vnStore, m_pCompiler->vnStore->VNConservativeNormalValue(expr->gtVNPair))) { // For XOR we only care about Log2 pattern for now, which never overflows. overflows = false; diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 93984f181e76b7..bbfc72574bde74 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9829,6 +9829,37 @@ bool ValueNumStore::GetVNFunc(ValueNum vn, VNFuncApp* funcApp) return false; } +//---------------------------------------------------------------------------------- +// IsBinFunc: A specialized version of GetVNFunc that checks if the given ValueNum +// is the given VNFunc with arity 2. If so, it returns the two operands. +// +// Arguments: +// vn - The ValueNum to check. +// func - The VNFunc to check for. +// op1 - The first operand (if not null). +// op2 - The second operand (if not null). +// +// Return Value: +// true if the ValueNum is the given VNFunc with arity 2, false otherwise. +// +bool ValueNumStore::IsBinFunc(ValueNum vn, VNFunc func, ValueNum* op1, ValueNum* op2) +{ + VNFuncApp funcApp; + if (GetVNFunc(vn, &funcApp) && (funcApp.m_func == func) && (funcApp.m_arity == 2)) + { + if (op1 != nullptr) + { + *op1 = funcApp.m_args[0]; + } + if (op2 != nullptr) + { + *op2 = funcApp.m_args[1]; + } + return true; + } + return false; +} + bool ValueNumStore::VNIsValid(ValueNum vn) { ChunkNum cn = GetChunkNum(vn); diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index a643789a2843ee..229c545a813a2c 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1408,6 +1408,9 @@ class ValueNumStore // the function application it represents; otherwise, return "false." bool GetVNFunc(ValueNum vn, VNFuncApp* funcApp); + // Returns "true" iff "vn" is a function application of the form "func(op1, op2)". + bool IsBinFunc(ValueNum vn, VNFunc func, ValueNum* op1 = nullptr, ValueNum* op2 = nullptr); + // Returns "true" iff "vn" is a valid value number -- one that has been previously returned. bool VNIsValid(ValueNum vn); From 44c430aff04efb8dea2d20835bfa86298efc3bab Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 22 Mar 2025 17:02:52 +0100 Subject: [PATCH 3/7] Fix compilation and arm --- src/coreclr/jit/rangecheck.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 3b1a28e25e65f8..179208744102b2 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1059,6 +1059,7 @@ void RangeCheck::MergeAssertion(BasicBlock* block, GenTree* op, Range* pRange DE // static bool IsLog2(ValueNumStore* vnStore, ValueNum vn, int* upperBound = nullptr) { +#if defined(FEATURE_HW_INTRINSICS) && (defined(TARGET_XARCH) || defined(TARGET_ARM64)) // First, look for "X ^ 31" or "X ^ 63" patterns... int xorBy; ValueNum op1, op2; @@ -1070,7 +1071,12 @@ static bool IsLog2(ValueNumStore* vnStore, ValueNum vn, int* upperBound = nullpt // Drop any integer cast if any, we're dealing with [0..63] range, any integer cast is redundant. vnStore->IsBinFunc(op1, VNF_Cast, &op1); - VNFunc lzcnFunc = (xorBy == 31) ? VNF_HWI_LZCNT_LeadingZeroCount : VNF_HWI_LZCNT_X64_LeadingZeroCount; + VNFunc lzcnFunc; +#ifdef TARGET_XARCH + lzcnFunc = (xorBy == 31) ? VNF_HWI_LZCNT_LeadingZeroCount : VNF_HWI_LZCNT_X64_LeadingZeroCount; +#else + lzcnFunc = (xorBy == 31) ? VNF_HWI_ArmBase_LeadingZeroCount : VNF_HWI_ArmBase_Arm64_LeadingZeroCount; +#endif if (vnStore->IsBinFunc(op1, lzcnFunc, &op1)) { // Last, check if the argument of the LZCNT32/LZCNT64 is "OR(..., 1)". @@ -1085,6 +1091,7 @@ static bool IsLog2(ValueNumStore* vnStore, ValueNum vn, int* upperBound = nullpt } } } +#endif return false; } From cdc449d8393350e39445f8c6a25c25f59faffe44 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 22 Mar 2025 19:38:01 +0100 Subject: [PATCH 4/7] clean up --- src/coreclr/jit/rangecheck.cpp | 44 +++++++++++++--------------------- src/coreclr/jit/valuenum.cpp | 6 ++--- src/coreclr/jit/valuenum.h | 31 +++++++++++++++++++++++- 3 files changed, 50 insertions(+), 31 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 179208744102b2..2d500bf7229adc 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1045,9 +1045,8 @@ void RangeCheck::MergeAssertion(BasicBlock* block, GenTree* op, Range* pRange DE } //------------------------------------------------------------------------ -// IsLog2: Determine if the value number is a log2 pattern, which is -// XOR(LZCNT32(OR(x, 1), 31) or XOR(LZCNT64(OR(x, 1), 63)). -// This represents what BitOperations.Log2/int.Log2/long.Log2 would do. +// IsLog2Pattern: Determine if the value number is a log2 pattern, which is +// "XOR(LZCNT32(OR(X, 1), 31)" or "XOR(LZCNT64(OR(X, 1), 63))". // // Arguments: // vnStore - the value number store @@ -1057,38 +1056,28 @@ void RangeCheck::MergeAssertion(BasicBlock* block, GenTree* op, Range* pRange DE // Return value: // true if the value number is a log2 pattern, false otherwise. // -static bool IsLog2(ValueNumStore* vnStore, ValueNum vn, int* upperBound = nullptr) +static bool IsLog2Pattern(ValueNumStore* vnStore, ValueNum vn, int* upperBound = nullptr) { #if defined(FEATURE_HW_INTRINSICS) && (defined(TARGET_XARCH) || defined(TARGET_ARM64)) - // First, look for "X ^ 31" or "X ^ 63" patterns... int xorBy; - ValueNum op1, op2; - if (vnStore->IsBinFunc(vn, VNF_XOR, &op1, &op2) && vnStore->IsVNIntegralConstant(op2, &xorBy) && - ((xorBy == 31) || (xorBy == 63))) + ValueNum op; + if (vnStore->IsVNBinFuncWithIntCon(vn, VNF_XOR, &op, &xorBy) && ((xorBy == 31) || (xorBy == 63))) { - // ...where that X has to be either LZCNT32 (in case of ^ 31) or LZCNT64 (in case of ^ 63) - // Drop any integer cast if any, we're dealing with [0..63] range, any integer cast is redundant. - vnStore->IsBinFunc(op1, VNF_Cast, &op1); + vnStore->IsVNBinFunc(op, VNF_Cast, &op); - VNFunc lzcnFunc; #ifdef TARGET_XARCH - lzcnFunc = (xorBy == 31) ? VNF_HWI_LZCNT_LeadingZeroCount : VNF_HWI_LZCNT_X64_LeadingZeroCount; + VNFunc lzcntFunc = (xorBy == 31) ? VNF_HWI_LZCNT_LeadingZeroCount : VNF_HWI_LZCNT_X64_LeadingZeroCount; #else - lzcnFunc = (xorBy == 31) ? VNF_HWI_ArmBase_LeadingZeroCount : VNF_HWI_ArmBase_Arm64_LeadingZeroCount; + VNFunc lzcntFunc = (xorBy == 31) ? VNF_HWI_ArmBase_LeadingZeroCount : VNF_HWI_ArmBase_Arm64_LeadingZeroCount; #endif - if (vnStore->IsBinFunc(op1, lzcnFunc, &op1)) + int orBy; + if (vnStore->IsVNBinFunc(op, lzcntFunc, &op) && vnStore->IsVNBinFuncWithIntCon(op, VNF_OR, &op, &orBy) && + (orBy == 1)) { - // Last, check if the argument of the LZCNT32/LZCNT64 is "OR(..., 1)". - int orBy; - if (vnStore->IsBinFunc(op1, VNF_OR, &op1, &op2) && vnStore->IsVNIntegralConstant(op2, &orBy) && (orBy == 1)) - { - if (upperBound != nullptr) - { - *upperBound = xorBy; - } - return true; - } + if (upperBound != nullptr) + *upperBound = xorBy; + return true; } } #endif @@ -1104,7 +1093,8 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool if (binop->OperIs(GT_XOR)) { int upperBound; - if (IsLog2(m_pCompiler->vnStore, m_pCompiler->vnStore->VNConservativeNormalValue(binop->gtVNPair), &upperBound)) + if (IsLog2Pattern(m_pCompiler->vnStore, m_pCompiler->vnStore->VNConservativeNormalValue(binop->gtVNPair), + &upperBound)) { assert(upperBound > 0); return Range(Limit(Limit::keConstant, 0), Limit(Limit::keConstant, upperBound)); @@ -1573,7 +1563,7 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Ran overflows = false; } else if (expr->OperIs(GT_XOR) && - IsLog2(m_pCompiler->vnStore, m_pCompiler->vnStore->VNConservativeNormalValue(expr->gtVNPair))) + IsLog2Pattern(m_pCompiler->vnStore, m_pCompiler->vnStore->VNConservativeNormalValue(expr->gtVNPair))) { // For XOR we only care about Log2 pattern for now, which never overflows. overflows = false; diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index bbfc72574bde74..b1b0ff07e3d54e 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9830,7 +9830,7 @@ bool ValueNumStore::GetVNFunc(ValueNum vn, VNFuncApp* funcApp) } //---------------------------------------------------------------------------------- -// IsBinFunc: A specialized version of GetVNFunc that checks if the given ValueNum +// IsVNBinFunc: A specialized version of GetVNFunc that checks if the given ValueNum // is the given VNFunc with arity 2. If so, it returns the two operands. // // Arguments: @@ -9840,9 +9840,9 @@ bool ValueNumStore::GetVNFunc(ValueNum vn, VNFuncApp* funcApp) // op2 - The second operand (if not null). // // Return Value: -// true if the ValueNum is the given VNFunc with arity 2, false otherwise. +// true if the given vn is the given VNFunc with two operands. // -bool ValueNumStore::IsBinFunc(ValueNum vn, VNFunc func, ValueNum* op1, ValueNum* op2) +bool ValueNumStore::IsVNBinFunc(ValueNum vn, VNFunc func, ValueNum* op1, ValueNum* op2) { VNFuncApp funcApp; if (GetVNFunc(vn, &funcApp) && (funcApp.m_func == func) && (funcApp.m_arity == 2)) diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 229c545a813a2c..dbfd8e8578593c 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1409,7 +1409,36 @@ class ValueNumStore bool GetVNFunc(ValueNum vn, VNFuncApp* funcApp); // Returns "true" iff "vn" is a function application of the form "func(op1, op2)". - bool IsBinFunc(ValueNum vn, VNFunc func, ValueNum* op1 = nullptr, ValueNum* op2 = nullptr); + bool IsVNBinFunc(ValueNum vn, VNFunc func, ValueNum* op1 = nullptr, ValueNum* op2 = nullptr); + + // Returns "true" iff "vn" is a function application of the form "func(op, cns)" + // the cns can be on the left side if the function is commutative. + template + bool IsVNBinFuncWithIntCon(ValueNum vn, VNFunc func, ValueNum* op, T* cns) + { + T opCns; + ValueNum op1, op2; + if (IsVNBinFunc(vn, func, &op1, &op2)) + { + if (IsVNIntegralConstant(op2, &opCns)) + { + if (op != nullptr) + *op = op1; + if (cns != nullptr) + *cns = opCns; + return true; + } + else if (VNFuncIsCommutative(func) && IsVNIntegralConstant(op1, &opCns)) + { + if (op != nullptr) + *op = op2; + if (cns != nullptr) + *cns = opCns; + return true; + } + } + return false; + } // Returns "true" iff "vn" is a valid value number -- one that has been previously returned. bool VNIsValid(ValueNum vn); From 4f613745e2bffdbeed7dfaa690b6733c981be1e5 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 22 Mar 2025 19:41:45 +0100 Subject: [PATCH 5/7] Cleanup --- src/coreclr/jit/rangecheck.cpp | 6 ++++-- src/coreclr/jit/valuenum.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 2d500bf7229adc..bde04f3a8ebc14 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1061,7 +1061,8 @@ static bool IsLog2Pattern(ValueNumStore* vnStore, ValueNum vn, int* upperBound = #if defined(FEATURE_HW_INTRINSICS) && (defined(TARGET_XARCH) || defined(TARGET_ARM64)) int xorBy; ValueNum op; - if (vnStore->IsVNBinFuncWithIntCon(vn, VNF_XOR, &op, &xorBy) && ((xorBy == 31) || (xorBy == 63))) + // First, see if it's "X ^ 31" or "X ^ 63". + if (vnStore->IsVNBinFuncWithConst(vn, VNF_XOR, &op, &xorBy) && ((xorBy == 31) || (xorBy == 63))) { // Drop any integer cast if any, we're dealing with [0..63] range, any integer cast is redundant. vnStore->IsVNBinFunc(op, VNF_Cast, &op); @@ -1071,8 +1072,9 @@ static bool IsLog2Pattern(ValueNumStore* vnStore, ValueNum vn, int* upperBound = #else VNFunc lzcntFunc = (xorBy == 31) ? VNF_HWI_ArmBase_LeadingZeroCount : VNF_HWI_ArmBase_Arm64_LeadingZeroCount; #endif + // Next, see if it's "LZCNT32(X | 1)" or "LZCNT64(X | 1)". int orBy; - if (vnStore->IsVNBinFunc(op, lzcntFunc, &op) && vnStore->IsVNBinFuncWithIntCon(op, VNF_OR, &op, &orBy) && + if (vnStore->IsVNBinFunc(op, lzcntFunc, &op) && vnStore->IsVNBinFuncWithConst(op, VNF_OR, &op, &orBy) && (orBy == 1)) { if (upperBound != nullptr) diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index dbfd8e8578593c..7dcd8581f0be47 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1414,7 +1414,7 @@ class ValueNumStore // Returns "true" iff "vn" is a function application of the form "func(op, cns)" // the cns can be on the left side if the function is commutative. template - bool IsVNBinFuncWithIntCon(ValueNum vn, VNFunc func, ValueNum* op, T* cns) + bool IsVNBinFuncWithConst(ValueNum vn, VNFunc func, ValueNum* op, T* cns) { T opCns; ValueNum op1, op2; From 73d002963dfca60880c0d936d2ae4d076c28de9c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 23 Mar 2025 01:11:10 +0100 Subject: [PATCH 6/7] move to vn --- src/coreclr/jit/rangecheck.cpp | 53 ++++------------------------------ src/coreclr/jit/valuenum.cpp | 49 +++++++++++++++++++++++++++++++ src/coreclr/jit/valuenum.h | 3 ++ 3 files changed, 58 insertions(+), 47 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index bde04f3a8ebc14..75cdc8c1e8264f 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1044,48 +1044,6 @@ void RangeCheck::MergeAssertion(BasicBlock* block, GenTree* op, Range* pRange DE } } -//------------------------------------------------------------------------ -// IsLog2Pattern: Determine if the value number is a log2 pattern, which is -// "XOR(LZCNT32(OR(X, 1), 31)" or "XOR(LZCNT64(OR(X, 1), 63))". -// -// Arguments: -// vnStore - the value number store -// vn - the value number to analyze -// upperBound - if not null, will be set to the upper bound of the log2 pattern (31 or 63) -// -// Return value: -// true if the value number is a log2 pattern, false otherwise. -// -static bool IsLog2Pattern(ValueNumStore* vnStore, ValueNum vn, int* upperBound = nullptr) -{ -#if defined(FEATURE_HW_INTRINSICS) && (defined(TARGET_XARCH) || defined(TARGET_ARM64)) - int xorBy; - ValueNum op; - // First, see if it's "X ^ 31" or "X ^ 63". - if (vnStore->IsVNBinFuncWithConst(vn, VNF_XOR, &op, &xorBy) && ((xorBy == 31) || (xorBy == 63))) - { - // Drop any integer cast if any, we're dealing with [0..63] range, any integer cast is redundant. - vnStore->IsVNBinFunc(op, VNF_Cast, &op); - -#ifdef TARGET_XARCH - VNFunc lzcntFunc = (xorBy == 31) ? VNF_HWI_LZCNT_LeadingZeroCount : VNF_HWI_LZCNT_X64_LeadingZeroCount; -#else - VNFunc lzcntFunc = (xorBy == 31) ? VNF_HWI_ArmBase_LeadingZeroCount : VNF_HWI_ArmBase_Arm64_LeadingZeroCount; -#endif - // Next, see if it's "LZCNT32(X | 1)" or "LZCNT64(X | 1)". - int orBy; - if (vnStore->IsVNBinFunc(op, lzcntFunc, &op) && vnStore->IsVNBinFuncWithConst(op, VNF_OR, &op, &orBy) && - (orBy == 1)) - { - if (upperBound != nullptr) - *upperBound = xorBy; - return true; - } - } -#endif - return false; -} - // Compute the range for a binary operation. Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool monIncreasing DEBUGARG(int indent)) { @@ -1095,8 +1053,8 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool if (binop->OperIs(GT_XOR)) { int upperBound; - if (IsLog2Pattern(m_pCompiler->vnStore, m_pCompiler->vnStore->VNConservativeNormalValue(binop->gtVNPair), - &upperBound)) + if (m_pCompiler->vnStore->IsVNLog2(m_pCompiler->vnStore->VNConservativeNormalValue(binop->gtVNPair), + &upperBound)) { assert(upperBound > 0); return Range(Limit(Limit::keConstant, 0), Limit(Limit::keConstant, upperBound)); @@ -1526,6 +1484,8 @@ bool RangeCheck::DoesOverflow(BasicBlock* block, GenTree* expr, const Range& ran bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Range& range) { + ValueNumStore* vnStore = m_pCompiler->vnStore; + JITDUMP("Does overflow [%06d]?\n", Compiler::dspTreeID(expr)); GetSearchPath()->Set(expr, block, SearchPath::Overwrite); @@ -1536,7 +1496,7 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Ran overflows = true; } // If the definition chain resolves to a constant, it doesn't overflow. - else if (m_pCompiler->vnStore->IsVNConstant(expr->gtVNPair.GetConservative())) + else if (vnStore->IsVNConstant(expr->gtVNPair.GetConservative())) { overflows = false; } @@ -1564,8 +1524,7 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Ran { overflows = false; } - else if (expr->OperIs(GT_XOR) && - IsLog2Pattern(m_pCompiler->vnStore, m_pCompiler->vnStore->VNConservativeNormalValue(expr->gtVNPair))) + else if (expr->OperIs(GT_XOR) && vnStore->IsVNLog2(m_pCompiler->vnStore->VNConservativeNormalValue(expr->gtVNPair))) { // For XOR we only care about Log2 pattern for now, which never overflows. overflows = false; diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index b1b0ff07e3d54e..98e807d140d31f 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -6571,6 +6571,48 @@ bool ValueNumStore::IsVNInt32Constant(ValueNum vn) return TypeOfVN(vn) == TYP_INT; } +//------------------------------------------------------------------------ +// IsVNLog2: Determine if the value number is a log2 pattern, which is +// "XOR(LZCNT32(OR(X, 1), 31)" or "XOR(LZCNT64(OR(X, 1), 63))". +// +// Arguments: +// vn - the value number to analyze +// upperBound - if not null, will be set to the upper bound of the log2 pattern (31 or 63) +// +// Return value: +// true if the value number is a log2 pattern, false otherwise. +// +bool ValueNumStore::IsVNLog2(ValueNum vn, int* upperBound) +{ +#if defined(FEATURE_HW_INTRINSICS) && (defined(TARGET_XARCH) || defined(TARGET_ARM64)) + int xorBy; + ValueNum op; + // First, see if it's "X ^ 31" or "X ^ 63". + if (IsVNBinFuncWithConst(vn, VNF_XOR, &op, &xorBy) && ((xorBy == 31) || (xorBy == 63))) + { + // Drop any integer cast if any, we're dealing with [0..63] range, any integer cast is redundant. + IsVNBinFunc(op, VNF_Cast, &op); + +#ifdef TARGET_XARCH + VNFunc lzcntFunc = (xorBy == 31) ? VNF_HWI_LZCNT_LeadingZeroCount : VNF_HWI_LZCNT_X64_LeadingZeroCount; +#else + VNFunc lzcntFunc = (xorBy == 31) ? VNF_HWI_ArmBase_LeadingZeroCount : VNF_HWI_ArmBase_Arm64_LeadingZeroCount; +#endif + // Next, see if it's "LZCNT32(X | 1)" or "LZCNT64(X | 1)". + int orBy; + if (IsVNBinFunc(op, lzcntFunc, &op) && IsVNBinFuncWithConst(op, VNF_OR, &op, &orBy) && (orBy == 1)) + { + if (upperBound != nullptr) + { + *upperBound = xorBy; + } + return true; + } + } +#endif + return false; +} + //------------------------------------------------------------------------ // IsVNNeverNegative: Determines if the given value number can never take on a negative value // in a signed context (i.e. when the most-significant bit represents signedness). @@ -6646,6 +6688,13 @@ bool ValueNumStore::IsVNNeverNegative(ValueNum vn) case VNF_HWI_ArmBase_Arm64_LeadingSignCount: return VNVisit::Continue; #endif + case VNF_XOR: + if (IsVNLog2(vn)) + { + return VNVisit::Continue; + } + break; + #endif // FEATURE_HW_INTRINSICS default: diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 7dcd8581f0be47..6753687f1feb79 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1055,6 +1055,9 @@ class ValueNumStore // Returns true if the VN represents a node that is never negative. bool IsVNNeverNegative(ValueNum vn); + // Returns true if the VN represents BitOperations.Log2 pattern + bool IsVNLog2(ValueNum vn, int* upperBound = nullptr); + typedef SmallHashTable CheckedBoundVNSet; // Returns true if the VN is known or likely to appear as the conservative value number From 7291f0ccc7c1f258f8b94d7593abd81a586452d0 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 23 Mar 2025 02:54:53 +0100 Subject: [PATCH 7/7] Remove redundant constant swap logic --- src/coreclr/jit/valuenum.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 98e807d140d31f..32d1ecc269472e 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2766,12 +2766,6 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V { std::swap(arg0VN, arg1VN); } - - // Try to keep constants on the right side. - if (IsVNConstant(arg0VN) && !IsVNConstant(arg1VN)) - { - std::swap(arg0VN, arg1VN); - } } // Have we already assigned a ValueNum for 'func'('arg0VN','arg1VN') ?