From 64f704210e734d17468ade1fe2154a7807a87695 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 17 Feb 2022 13:12:40 -0800 Subject: [PATCH 01/31] Initial work for ARM64 mod optimization --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/gentree.h | 7 +++++ src/coreclr/jit/morph.cpp | 53 +++++++++++++++++++++++++++++++++++--- 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index fa5bd6087220c..9e8db661d9ce6 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6431,6 +6431,7 @@ class Compiler GenTree* fgPropagateCommaThrow(GenTree* parent, GenTreeOp* commaThrow, GenTreeFlags precedingSideEffects); GenTree* fgMorphRetInd(GenTreeUnOp* tree); GenTree* fgMorphModToSubMulDiv(GenTreeOp* tree); + GenTree* fgMorphUModToAndSub(GenTreeOp* tree); GenTree* fgMorphSmpOpOptional(GenTreeOp* tree); GenTree* fgMorphMultiOp(GenTreeMultiOp* multiOp); GenTree* fgMorphConst(GenTree* tree); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index a9dcd48c71ef3..e814b0c665878 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -2107,6 +2107,8 @@ struct GenTree inline bool IsCnsIntOrI() const; + inline bool IsCnsIntPow2() const; + inline bool IsIntegralConst() const; inline bool IsIntCnsFitsInI32(); // Constant fits in INT32 @@ -8304,6 +8306,11 @@ inline bool GenTree::IsCnsIntOrI() const return (gtOper == GT_CNS_INT); } +inline bool GenTree::IsCnsIntPow2() const +{ + return IsCnsIntOrI() && isPow2(AsIntConCommon()->IconValue()); +} + inline bool GenTree::IsIntegralConst() const { #ifdef TARGET_64BIT diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index ed702b872e8e8..628a86aec38c5 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11418,10 +11418,18 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // in Morph we can't guarantee that `a` won't be transformed into a constant, // so can't guarantee that lower will be able to do this optimization. { - // Do "a % b = a - (a / b) * b" morph always, see TODO before this block. - bool doMorphModToSubMulDiv = true; + bool doMorphModToAnd = false; - if (doMorphModToSubMulDiv) + if(oper == GT_UMOD && op2->IsCnsIntPow2()) + { + assert(!optValnumCSE_phase); + + tree = fgMorphUModToAndSub(tree->AsOp()); + op1 = tree->AsOp()->gtOp1; + op2 = tree->AsOp()->gtOp2; + } + // Do "a % b = a - (a / b) * b" morph always, see TODO before this block. + else { assert(!optValnumCSE_phase); @@ -14594,6 +14602,45 @@ GenTree* Compiler::fgMorphModToSubMulDiv(GenTreeOp* tree) return sub; } +//------------------------------------------------------------------------ +// fgMorphModToSubMulDiv: Transform a % b into the equivalent a - (a / b) * b +// (see ECMA III 3.55 and III.3.56). +// +// Arguments: +// tree - The GT_MOD/GT_UMOD tree to morph +// +// Returns: +// The morphed tree +// +// Notes: +// For ARM64 we don't have a remainder instruction so this transform is +// always done. For XARCH this transform is done if we know that magic +// division will be used, in that case this transform allows CSE to +// eliminate the redundant div from code like "x = a / 3; y = a % 3;". +// +GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) +{ + JITDUMP("\nMorphing UMOD [%06u] to And/Sub\n", dspTreeID(tree)); + + if (tree->OperGet() != GT_UMOD) + { + noway_assert(!"Illegal gtOper in fgMorphUModToAndSub"); + } + + var_types type = tree->gtType; + + GenTree* const copyOfNumeratorValue = fgMakeMultiUse(&tree->gtOp1); + GenTree* const copyOfDenominatorValue = fgMakeMultiUse(&tree->gtOp2); + GenTree* const sub = gtNewOperNode(GT_SUB, type, copyOfDenominatorValue, gtNewOneConNode(type)); + GenTree* const and = gtNewOperNode(GT_AND, type, copyOfNumeratorValue, sub); + +#ifdef DEBUG + and->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; +#endif + + return and; +} + //------------------------------------------------------------------------------ // fgOperIsBitwiseRotationRoot : Check if the operation can be a root of a bitwise rotation tree. // From d1dce26c19b31964c8e40ddc1ceb27ba7c22fd53 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 17 Feb 2022 16:15:56 -0800 Subject: [PATCH 02/31] Updated comment --- src/coreclr/jit/morph.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 628a86aec38c5..737571c1f864f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14603,20 +14603,19 @@ GenTree* Compiler::fgMorphModToSubMulDiv(GenTreeOp* tree) } //------------------------------------------------------------------------ -// fgMorphModToSubMulDiv: Transform a % b into the equivalent a - (a / b) * b -// (see ECMA III 3.55 and III.3.56). +// fgMorphUModToAndSub: Transform a % b into the equivalent a & (b - 1). +// '%' must be unsigned (GT_UMOD). +// 'a' and 'b' must be integers. +// 'b' must be a constant and a power of two. // // Arguments: -// tree - The GT_MOD/GT_UMOD tree to morph +// tree - The GT_UMOD tree to morph // // Returns: // The morphed tree // // Notes: -// For ARM64 we don't have a remainder instruction so this transform is -// always done. For XARCH this transform is done if we know that magic -// division will be used, in that case this transform allows CSE to -// eliminate the redundant div from code like "x = a / 3; y = a % 3;". +// For ARM64 this is an optimization versus calling fgMorphModToSubMulDiv. // GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) { @@ -14627,6 +14626,8 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) noway_assert(!"Illegal gtOper in fgMorphUModToAndSub"); } + assert(tree->gtOp2->IsCnsIntPow2()); + var_types type = tree->gtType; GenTree* const copyOfNumeratorValue = fgMakeMultiUse(&tree->gtOp1); From a3fbe54803923e66bdd0f8a023e2d2a270a8ff9b Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 17 Feb 2022 16:21:44 -0800 Subject: [PATCH 03/31] Updated comment --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 737571c1f864f..7b6d2c148fe64 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11408,7 +11408,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // // a % b = a - (a / b) * b; // - // TODO: there are special cases where it can be done better, for example + // There are special cases where it can be done better, for example // when the modulo operation is unsigned and the divisor is a // integer constant power of two. In this case, we can make the transform: // From 729057d0cae621a4e5f3dc72399b09c83b64a29c Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 17 Feb 2022 16:25:02 -0800 Subject: [PATCH 04/31] Updated comment --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 7b6d2c148fe64..5a019350a00df 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11428,7 +11428,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; } - // Do "a % b = a - (a / b) * b" morph always, see TODO before this block. + // Do "a % b = a - (a / b) * b" morph. else { assert(!optValnumCSE_phase); From c2eee76a8930145d185940a9c24aef9a2f13c45b Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 17 Feb 2022 17:40:29 -0800 Subject: [PATCH 05/31] Fixing build --- src/coreclr/jit/morph.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 5a019350a00df..0fafe35430b8f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14633,13 +14633,13 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) GenTree* const copyOfNumeratorValue = fgMakeMultiUse(&tree->gtOp1); GenTree* const copyOfDenominatorValue = fgMakeMultiUse(&tree->gtOp2); GenTree* const sub = gtNewOperNode(GT_SUB, type, copyOfDenominatorValue, gtNewOneConNode(type)); - GenTree* const and = gtNewOperNode(GT_AND, type, copyOfNumeratorValue, sub); + GenTree* const andSub = gtNewOperNode(GT_AND, type, copyOfNumeratorValue, sub); #ifdef DEBUG - and->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; + andSub->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; #endif - return and; + return andSub; } //------------------------------------------------------------------------------ From ca333e0ec87fab3f3f8f3d3dbf60d065ce59eff7 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 17 Feb 2022 17:41:45 -0800 Subject: [PATCH 06/31] Remove uneeded var --- src/coreclr/jit/morph.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 0fafe35430b8f..6640e40a2d3ac 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11418,8 +11418,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // in Morph we can't guarantee that `a` won't be transformed into a constant, // so can't guarantee that lower will be able to do this optimization. { - bool doMorphModToAnd = false; - if(oper == GT_UMOD && op2->IsCnsIntPow2()) { assert(!optValnumCSE_phase); From 7fc88edfbc7a19f6a53bbc3fa7a21fb6c57e7ecf Mon Sep 17 00:00:00 2001 From: Will Smith Date: Wed, 23 Feb 2022 15:02:17 -0800 Subject: [PATCH 07/31] Use '%' morph logic for both x64/arm64 --- src/coreclr/jit/gentree.h | 14 +++++----- src/coreclr/jit/morph.cpp | 55 +++++++-------------------------------- 2 files changed, 16 insertions(+), 53 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index e814b0c665878..3cd31a1447471 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -2107,10 +2107,10 @@ struct GenTree inline bool IsCnsIntOrI() const; - inline bool IsCnsIntPow2() const; - inline bool IsIntegralConst() const; + inline bool IsIntegralConstPow2() const; + inline bool IsIntCnsFitsInI32(); // Constant fits in INT32 inline bool IsCnsFltOrDbl() const; @@ -8306,11 +8306,6 @@ inline bool GenTree::IsCnsIntOrI() const return (gtOper == GT_CNS_INT); } -inline bool GenTree::IsCnsIntPow2() const -{ - return IsCnsIntOrI() && isPow2(AsIntConCommon()->IconValue()); -} - inline bool GenTree::IsIntegralConst() const { #ifdef TARGET_64BIT @@ -8320,6 +8315,11 @@ inline bool GenTree::IsIntegralConst() const #endif // !TARGET_64BIT } +inline bool GenTree::IsIntegralConstPow2() const +{ + return IsIntegralConst() && isPow2(AsIntConCommon()->IconValue()); +} + // Is this node an integer constant that fits in a 32-bit signed integer (INT32) inline bool GenTree::IsIntCnsFitsInI32() { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 6640e40a2d3ac..449c52e948883 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11401,64 +11401,27 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) #endif #endif // !TARGET_64BIT -#ifdef TARGET_ARM64 - // For ARM64 we don't have a remainder instruction, - // The architecture manual suggests the following transformation to - // generate code for such operator: - // - // a % b = a - (a / b) * b; - // - // There are special cases where it can be done better, for example - // when the modulo operation is unsigned and the divisor is a - // integer constant power of two. In this case, we can make the transform: - // - // a % b = a & (b - 1); - // - // Lower supports it for all cases except when `a` is constant, but - // in Morph we can't guarantee that `a` won't be transformed into a constant, - // so can't guarantee that lower will be able to do this optimization. + if (!optValnumCSE_phase) { - if(oper == GT_UMOD && op2->IsCnsIntPow2()) + // a % b = a & (b - 1); + if(oper == GT_UMOD && op2->IsIntegralConstPow2()) { - assert(!optValnumCSE_phase); - tree = fgMorphUModToAndSub(tree->AsOp()); op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; } - // Do "a % b = a - (a / b) * b" morph. + // a % b = a - (a / b) * b; +#ifdef TARGET_ARM64 else - { - assert(!optValnumCSE_phase); - - tree = fgMorphModToSubMulDiv(tree->AsOp()); - op1 = tree->AsOp()->gtOp1; - op2 = tree->AsOp()->gtOp2; - } - } -#else // !TARGET_ARM64 - // If b is not a power of 2 constant then lowering replaces a % b - // with a - (a / b) * b and applies magic division optimization to - // a / b. The code may already contain an a / b expression (e.g. - // x = a / 10; y = a % 10;) and then we end up with redundant code. - // If we convert % to / here we give CSE the opportunity to eliminate - // the redundant division. If there's no redundant division then - // nothing is lost, lowering would have done this transform anyway. - - if (!optValnumCSE_phase && ((tree->OperGet() == GT_MOD) && op2->IsIntegralConst())) - { - ssize_t divisorValue = op2->AsIntCon()->IconValue(); - size_t absDivisorValue = (divisorValue == SSIZE_T_MIN) ? static_cast(divisorValue) - : static_cast(abs(divisorValue)); - - if (!isPow2(absDivisorValue)) +#else + else if (oper == GT_MOD && !op2->IsIntegralConstPow2()) +#endif { tree = fgMorphModToSubMulDiv(tree->AsOp()); op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; } } -#endif // !TARGET_ARM64 break; USE_HELPER_FOR_ARITH: @@ -14624,7 +14587,7 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) noway_assert(!"Illegal gtOper in fgMorphUModToAndSub"); } - assert(tree->gtOp2->IsCnsIntPow2()); + assert(tree->gtOp2->IsIntegralConstPow2()); var_types type = tree->gtType; From 6656763aa9d1742d3373e7e0e016495b83889654 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Wed, 23 Feb 2022 18:41:11 -0800 Subject: [PATCH 08/31] Adding back in divisor check for x64 --- src/coreclr/jit/gentree.h | 14 ++++++++++++++ src/coreclr/jit/morph.cpp | 4 ++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index a4df6bd1bcfc3..ec3724e232dad 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -2111,6 +2111,8 @@ struct GenTree inline bool IsIntegralConstPow2() const; + inline bool IsIntegralConstAbsPow2() const; + inline bool IsIntCnsFitsInI32(); // Constant fits in INT32 inline bool IsCnsFltOrDbl() const; @@ -8336,6 +8338,18 @@ inline bool GenTree::IsIntegralConstPow2() const return IsIntegralConst() && isPow2(AsIntConCommon()->IconValue()); } +inline bool GenTree::IsIntegralConstAbsPow2() const +{ + if (IsIntegralConst()) + { + ssize_t value = AsIntConCommon()->IconValue(); + size_t absValue = (value == SSIZE_T_MIN) ? static_cast(value) : static_cast(abs(value)); + return isPow2(absValue); + } + + return false; +} + // Is this node an integer constant that fits in a 32-bit signed integer (INT32) inline bool GenTree::IsIntCnsFitsInI32() { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 88a5ad492e07f..69c3b6cc667a9 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11403,6 +11403,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (!optValnumCSE_phase) { +#ifdef TARGET_ARM64 // a % b = a & (b - 1); if(oper == GT_UMOD && op2->IsIntegralConstPow2()) { @@ -11411,10 +11412,9 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op2 = tree->AsOp()->gtOp2; } // a % b = a - (a / b) * b; -#ifdef TARGET_ARM64 else #else - else if (oper == GT_MOD && !op2->IsIntegralConstPow2()) + if (oper == GT_MOD && !op2->IsIntegralConstAbsPow2()) #endif { tree = fgMorphModToSubMulDiv(tree->AsOp()); From cfa9805fa7668ac5fa4db848347fdccd8f438fe4 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 25 Feb 2022 11:31:27 -0800 Subject: [PATCH 09/31] Formatting --- src/coreclr/jit/gentree.h | 2 +- src/coreclr/jit/morph.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index ec3724e232dad..86eb3986d5dc7 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8342,7 +8342,7 @@ inline bool GenTree::IsIntegralConstAbsPow2() const { if (IsIntegralConst()) { - ssize_t value = AsIntConCommon()->IconValue(); + ssize_t value = AsIntConCommon()->IconValue(); size_t absValue = (value == SSIZE_T_MIN) ? static_cast(value) : static_cast(abs(value)); return isPow2(absValue); } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 69c3b6cc667a9..4c542bbf34b10 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11405,7 +11405,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) { #ifdef TARGET_ARM64 // a % b = a & (b - 1); - if(oper == GT_UMOD && op2->IsIntegralConstPow2()) + if (oper == GT_UMOD && op2->IsIntegralConstPow2()) { tree = fgMorphUModToAndSub(tree->AsOp()); op1 = tree->AsOp()->gtOp1; From e0585532e15b37c13ec55d17988a416b966d1f30 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 25 Feb 2022 11:48:20 -0800 Subject: [PATCH 10/31] Update comments --- src/coreclr/jit/gentree.h | 20 ++++++++++++++++++++ src/coreclr/jit/morph.cpp | 11 +++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 86eb3986d5dc7..5791d69b3468f 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8333,11 +8333,31 @@ inline bool GenTree::IsIntegralConst() const #endif // !TARGET_64BIT } +//------------------------------------------------------------------------- +// IsIntegralConstPow2: Determines whether the node is an integral constant +// of the power of 2. +// +// Arguments: +// None +// +// Return Value: +// Returns true if this GenTree is an integral constant of the power +// of 2. inline bool GenTree::IsIntegralConstPow2() const { return IsIntegralConst() && isPow2(AsIntConCommon()->IconValue()); } +//------------------------------------------------------------------------- +// IsIntegralConstAbsPow2: Determines whether the absolute value of +// an integral constant is the power of 2. +// +// Arguments: +// None +// +// Return Value: +// Returns true if the absolute value of a GenTree's integral constant +// is the power of 2. inline bool GenTree::IsIntegralConstAbsPow2() const { if (IsIntegralConst()) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 4c542bbf34b10..1b59662e097a4 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11404,16 +11404,23 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (!optValnumCSE_phase) { #ifdef TARGET_ARM64 - // a % b = a & (b - 1); + // Transformation: a % b = a & (b - 1); if (oper == GT_UMOD && op2->IsIntegralConstPow2()) { tree = fgMorphUModToAndSub(tree->AsOp()); op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; } - // a % b = a - (a / b) * b; + // Transformation: a % b = a - (a / b) * b; + // ARM64 architecture manual suggests this transformation + // for the mod operator. else #else + // X64 only applies this transformation if we know + // that magic division will be used - which is determined + // when 'b' is not a power of 2 constant and mod operator is signed. + // Lowering for X64 does this optimization already, + // but is also done here to take advantage of CSE. if (oper == GT_MOD && !op2->IsIntegralConstAbsPow2()) #endif { From b950ab4662a5d1944285fddcc16e84026cb3ce20 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 25 Feb 2022 11:49:07 -0800 Subject: [PATCH 11/31] Update comments --- src/coreclr/jit/morph.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 1b59662e097a4..cdde2302981f2 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11416,10 +11416,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // for the mod operator. else #else - // X64 only applies this transformation if we know + // XARCH only applies this transformation if we know // that magic division will be used - which is determined // when 'b' is not a power of 2 constant and mod operator is signed. - // Lowering for X64 does this optimization already, + // Lowering for XARCH does this optimization already, // but is also done here to take advantage of CSE. if (oper == GT_MOD && !op2->IsIntegralConstAbsPow2()) #endif From bcab4617af191e86558691cad1f63c4f64ddd79c Mon Sep 17 00:00:00 2001 From: Will Smith Date: Tue, 1 Mar 2022 15:37:44 -0800 Subject: [PATCH 12/31] Fixing --- src/coreclr/jit/morph.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 5fbd96ccaea83..050351cfc474e 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11403,7 +11403,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (!optValnumCSE_phase) { -#ifdef TARGET_ARM64 // Transformation: a % b = a & (b - 1); if (oper == GT_UMOD && op2->IsIntegralConstPow2()) { @@ -11411,9 +11410,11 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; } +#ifdef TARGET_ARM64 // Transformation: a % b = a - (a / b) * b; // ARM64 architecture manual suggests this transformation // for the mod operator. + else #else // XARCH only applies this transformation if we know @@ -14744,15 +14745,15 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) var_types type = tree->gtType; - GenTree* const copyOfNumeratorValue = fgMakeMultiUse(&tree->gtOp1); - GenTree* const copyOfDenominatorValue = fgMakeMultiUse(&tree->gtOp2); - GenTree* const sub = gtNewOperNode(GT_SUB, type, copyOfDenominatorValue, gtNewOneConNode(type)); - GenTree* const andSub = gtNewOperNode(GT_AND, type, copyOfNumeratorValue, sub); + GenTree* const sub = gtNewOperNode(GT_SUB, type, tree->gtOp2, gtNewOneConNode(type)); + GenTree* const andSub = gtNewOperNode(GT_AND, type, tree->gtOp1, sub); #ifdef DEBUG andSub->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; #endif + DEBUG_DESTROY_NODE(tree); + return andSub; } From ba5b0eebfe2c7391ddc7ca7e5c962c89b82f1819 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Tue, 1 Mar 2022 15:47:19 -0800 Subject: [PATCH 13/31] Updated comment --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 050351cfc474e..12a5b125dcbd1 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14730,7 +14730,7 @@ GenTree* Compiler::fgMorphModToSubMulDiv(GenTreeOp* tree) // The morphed tree // // Notes: -// For ARM64 this is an optimization versus calling fgMorphModToSubMulDiv. +// This is more optimized than calling fgMorphModToSubMulDiv. // GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) { From 76af344ea1b76cded2eaf524594662d6fc0ae3ec Mon Sep 17 00:00:00 2001 From: Will Smith Date: Tue, 1 Mar 2022 15:50:03 -0800 Subject: [PATCH 14/31] Updated comment --- src/coreclr/jit/morph.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 12a5b125dcbd1..5c94228a828a8 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11403,15 +11403,14 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (!optValnumCSE_phase) { - // Transformation: a % b = a & (b - 1); if (oper == GT_UMOD && op2->IsIntegralConstPow2()) { + // Transformation: a % b = a & (b - 1); tree = fgMorphUModToAndSub(tree->AsOp()); op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; } #ifdef TARGET_ARM64 - // Transformation: a % b = a - (a / b) * b; // ARM64 architecture manual suggests this transformation // for the mod operator. @@ -11425,6 +11424,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (oper == GT_MOD && !op2->IsIntegralConstAbsPow2()) #endif { + // Transformation: a % b = a - (a / b) * b; tree = fgMorphModToSubMulDiv(tree->AsOp()); op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; From c7f6cb9d43253f608cd96692716242f07e286168 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Wed, 2 Mar 2022 13:18:16 -0800 Subject: [PATCH 15/31] Tweaking x64 transformation logic for the mod opt --- src/coreclr/jit/morph.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 5c94228a828a8..816f0effaf0fd 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11403,7 +11403,11 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (!optValnumCSE_phase) { +#ifdef TARGET_ARM64 if (oper == GT_UMOD && op2->IsIntegralConstPow2()) +#else + if (oper == GT_UMOD && !op2->IsIntegralConstAbsPow2()) +#endif { // Transformation: a % b = a & (b - 1); tree = fgMorphUModToAndSub(tree->AsOp()); @@ -11413,7 +11417,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) #ifdef TARGET_ARM64 // ARM64 architecture manual suggests this transformation // for the mod operator. - else #else // XARCH only applies this transformation if we know @@ -11421,7 +11424,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // when 'b' is not a power of 2 constant and mod operator is signed. // Lowering for XARCH does this optimization already, // but is also done here to take advantage of CSE. - if (oper == GT_MOD && !op2->IsIntegralConstAbsPow2()) + else if (oper == GT_MOD && !op2->IsIntegralConstAbsPow2()) #endif { // Transformation: a % b = a - (a / b) * b; From dee80b5cc75f3bd0e3ec01f6be6de4ea245689a8 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Wed, 2 Mar 2022 13:20:06 -0800 Subject: [PATCH 16/31] Tweaking x64 transformation logic for the mod opt --- src/coreclr/jit/morph.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 816f0effaf0fd..ca0b283936d56 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11405,16 +11405,12 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) { #ifdef TARGET_ARM64 if (oper == GT_UMOD && op2->IsIntegralConstPow2()) -#else - if (oper == GT_UMOD && !op2->IsIntegralConstAbsPow2()) -#endif { // Transformation: a % b = a & (b - 1); tree = fgMorphUModToAndSub(tree->AsOp()); op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; } -#ifdef TARGET_ARM64 // ARM64 architecture manual suggests this transformation // for the mod operator. else @@ -11424,7 +11420,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // when 'b' is not a power of 2 constant and mod operator is signed. // Lowering for XARCH does this optimization already, // but is also done here to take advantage of CSE. - else if (oper == GT_MOD && !op2->IsIntegralConstAbsPow2()) + if (oper == GT_MOD && !op2->IsIntegralConstAbsPow2()) #endif { // Transformation: a % b = a - (a / b) * b; From 1fac071b15ecb7c8cab86e544afc675c2418a51d Mon Sep 17 00:00:00 2001 From: Will Smith Date: Wed, 2 Mar 2022 18:26:10 -0800 Subject: [PATCH 17/31] Using IntCon --- src/coreclr/jit/gentree.h | 4 ++-- src/coreclr/jit/morph.cpp | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 4ea6c036c3da6..e5605be882e65 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8358,7 +8358,7 @@ inline bool GenTree::IsIntegralConst() const // of 2. inline bool GenTree::IsIntegralConstPow2() const { - return IsIntegralConst() && isPow2(AsIntConCommon()->IconValue()); + return IsIntegralConst() && isPow2(AsIntCon()->IconValue()); } //------------------------------------------------------------------------- @@ -8375,7 +8375,7 @@ inline bool GenTree::IsIntegralConstAbsPow2() const { if (IsIntegralConst()) { - ssize_t value = AsIntConCommon()->IconValue(); + ssize_t value = AsIntCon()->IconValue(); size_t absValue = (value == SSIZE_T_MIN) ? static_cast(value) : static_cast(abs(value)); return isPow2(absValue); } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index ca0b283936d56..abce2cd444dca 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11411,6 +11411,9 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; } + else if (oper == GT_MOD && op2->IsIntegralConstPow2()) + { + } // ARM64 architecture manual suggests this transformation // for the mod operator. else From 372bcf3370e7455c9b28e376ed9b29ad00588a00 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Wed, 2 Mar 2022 18:33:48 -0800 Subject: [PATCH 18/31] Fixed build --- src/coreclr/jit/morph.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index abce2cd444dca..ca0b283936d56 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11411,9 +11411,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op1 = tree->AsOp()->gtOp1; op2 = tree->AsOp()->gtOp2; } - else if (oper == GT_MOD && op2->IsIntegralConstPow2()) - { - } // ARM64 architecture manual suggests this transformation // for the mod operator. else From 880905886a4be9487aaa8ebfb3caba6d77cc4f1a Mon Sep 17 00:00:00 2001 From: Will Smith Date: Wed, 2 Mar 2022 18:39:02 -0800 Subject: [PATCH 19/31] Minor tweak --- src/coreclr/jit/codegenarm64.cpp | 12 ++++++++++++ src/coreclr/jit/morph.cpp | 4 ++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index 0c41d89a860d5..87e90a5a64515 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -3022,6 +3022,18 @@ void CodeGen::genCodeForNegNot(GenTree* tree) regNumber targetReg = tree->GetRegNum(); instruction ins = genGetInsForOper(tree->OperGet(), targetType); + if ((tree->gtFlags & GTF_SET_FLAGS) != 0) + { + switch (tree->OperGet()) + { + case GT_NEG: + ins = INS_negs; + break; + default: + noway_assert(!"Unexpected UnaryOp with GTF_SET_FLAGS set"); + } + } + // The arithmetic node must be sitting in a register (since it's not contained) assert(!tree->isContained()); // The dst can only be a register. diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index ca0b283936d56..bae316a105a3f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11404,7 +11404,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (!optValnumCSE_phase) { #ifdef TARGET_ARM64 - if (oper == GT_UMOD && op2->IsIntegralConstPow2()) + if ((tree->OperGet() == GT_UMOD) && op2->IsIntegralConstPow2()) { // Transformation: a % b = a & (b - 1); tree = fgMorphUModToAndSub(tree->AsOp()); @@ -11420,7 +11420,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // when 'b' is not a power of 2 constant and mod operator is signed. // Lowering for XARCH does this optimization already, // but is also done here to take advantage of CSE. - if (oper == GT_MOD && !op2->IsIntegralConstAbsPow2()) + if ((tree->OperGet() == GT_MOD) && !op2->IsIntegralConstAbsPow2()) #endif { // Transformation: a % b = a - (a / b) * b; From b16d38194cd34122b9ee22108bc45b31183514d7 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Wed, 2 Mar 2022 23:04:45 -0800 Subject: [PATCH 20/31] Fixing x64 diffs --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index bae316a105a3f..5c6fbcdb02ca2 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11420,7 +11420,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // when 'b' is not a power of 2 constant and mod operator is signed. // Lowering for XARCH does this optimization already, // but is also done here to take advantage of CSE. - if ((tree->OperGet() == GT_MOD) && !op2->IsIntegralConstAbsPow2()) + if ((tree->OperGet() == GT_MOD) && op2->IsIntegralConst() && !op2->IsIntegralConstAbsPow2()) #endif { // Transformation: a % b = a - (a / b) * b; From 9235f923e879efaa106e06419a92eda0519eb6df Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 3 Mar 2022 10:25:31 -0800 Subject: [PATCH 21/31] Removing flag set --- src/coreclr/jit/codegenarm64.cpp | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index 87e90a5a64515..0c41d89a860d5 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -3022,18 +3022,6 @@ void CodeGen::genCodeForNegNot(GenTree* tree) regNumber targetReg = tree->GetRegNum(); instruction ins = genGetInsForOper(tree->OperGet(), targetType); - if ((tree->gtFlags & GTF_SET_FLAGS) != 0) - { - switch (tree->OperGet()) - { - case GT_NEG: - ins = INS_negs; - break; - default: - noway_assert(!"Unexpected UnaryOp with GTF_SET_FLAGS set"); - } - } - // The arithmetic node must be sitting in a register (since it's not contained) assert(!tree->isContained()); // The dst can only be a register. From 36b1e6a40d8cb50fa59d5c4bab1f620e92946a7e Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 10 Mar 2022 10:42:49 -0800 Subject: [PATCH 22/31] Feedback --- src/coreclr/jit/gentree.h | 25 +++++++------------------ src/coreclr/jit/morph.cpp | 18 ++++++------------ 2 files changed, 13 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 99d90b923a1d3..e63a9edaa595d 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -2110,8 +2110,6 @@ struct GenTree inline bool IsIntegralConst() const; - inline bool IsIntegralConstPow2() const; - inline bool IsIntegralConstAbsPow2() const; inline bool IsIntCnsFitsInI32(); // Constant fits in INT32 @@ -8347,21 +8345,6 @@ inline bool GenTree::IsIntegralConst() const #endif // !TARGET_64BIT } -//------------------------------------------------------------------------- -// IsIntegralConstPow2: Determines whether the node is an integral constant -// of the power of 2. -// -// Arguments: -// None -// -// Return Value: -// Returns true if this GenTree is an integral constant of the power -// of 2. -inline bool GenTree::IsIntegralConstPow2() const -{ - return IsIntegralConst() && isPow2(AsIntCon()->IconValue()); -} - //------------------------------------------------------------------------- // IsIntegralConstAbsPow2: Determines whether the absolute value of // an integral constant is the power of 2. @@ -8372,11 +8355,17 @@ inline bool GenTree::IsIntegralConstPow2() const // Return Value: // Returns true if the absolute value of a GenTree's integral constant // is the power of 2. +// +// Notes: +// Integral constant nodes store its value in signed form. +// Because this function takes the absolute value of the constant's value, +// this should handle cases where an unsigned-int was logically used in +// user code. inline bool GenTree::IsIntegralConstAbsPow2() const { if (IsIntegralConst()) { - ssize_t value = AsIntCon()->IconValue(); + ssize_t value = AsIntConCommon()->IntegralValue(); size_t absValue = (value == SSIZE_T_MIN) ? static_cast(value) : static_cast(abs(value)); return isPow2(absValue); } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 5bfdc2239c721..853cc02b11afe 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11399,7 +11399,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (!optValnumCSE_phase) { #ifdef TARGET_ARM64 - if ((tree->OperGet() == GT_UMOD) && op2->IsIntegralConstPow2()) + if (tree->OperIs(GT_UMOD) && op2->IsIntegralConstAbsPow2()) { // Transformation: a % b = a & (b - 1); tree = fgMorphUModToAndSub(tree->AsOp()); @@ -11415,7 +11415,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // when 'b' is not a power of 2 constant and mod operator is signed. // Lowering for XARCH does this optimization already, // but is also done here to take advantage of CSE. - if ((tree->OperGet() == GT_MOD) && op2->IsIntegralConst() && !op2->IsIntegralConstAbsPow2()) + if (tree->OperIs(GT_MOD) && op2->IsIntegralConst() && !op2->IsIntegralConstAbsPow2()) #endif { // Transformation: a % b = a - (a / b) * b; @@ -14733,21 +14733,15 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) { JITDUMP("\nMorphing UMOD [%06u] to And/Sub\n", dspTreeID(tree)); - if (tree->OperGet() != GT_UMOD) - { - noway_assert(!"Illegal gtOper in fgMorphUModToAndSub"); - } - - assert(tree->gtOp2->IsIntegralConstPow2()); + assert(tree->OperIs(GT_UMOD)); + assert(tree->gtOp2->IsIntegralConstAbsPow2()); - var_types type = tree->gtType; + var_types type = tree->TypeGet(); GenTree* const sub = gtNewOperNode(GT_SUB, type, tree->gtOp2, gtNewOneConNode(type)); GenTree* const andSub = gtNewOperNode(GT_AND, type, tree->gtOp1, sub); -#ifdef DEBUG - andSub->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; -#endif + INDEBUG(andSub->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); DEBUG_DESTROY_NODE(tree); From 964b4267c0246f7a03fcd59e9fd0fbdae22b1a20 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 10 Mar 2022 11:18:28 -0800 Subject: [PATCH 23/31] Fixing build --- src/coreclr/jit/gentree.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index e63a9edaa595d..e9a0d0012b1b9 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8365,9 +8365,7 @@ inline bool GenTree::IsIntegralConstAbsPow2() const { if (IsIntegralConst()) { - ssize_t value = AsIntConCommon()->IntegralValue(); - size_t absValue = (value == SSIZE_T_MIN) ? static_cast(value) : static_cast(abs(value)); - return isPow2(absValue); + return isPow2(abs(AsIntConCommon()->IntegralValue())); } return false; From ec8246a68ea5343c5f77e7aba220406397f32321 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 10 Mar 2022 11:25:33 -0800 Subject: [PATCH 24/31] Feedback --- src/coreclr/jit/gentree.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index e9a0d0012b1b9..b8e4958cfd7a7 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8349,9 +8349,6 @@ inline bool GenTree::IsIntegralConst() const // IsIntegralConstAbsPow2: Determines whether the absolute value of // an integral constant is the power of 2. // -// Arguments: -// None -// // Return Value: // Returns true if the absolute value of a GenTree's integral constant // is the power of 2. From 27c3894f28e85ad3ff0809d269b22e4fd58b1ef5 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 11 Mar 2022 12:22:20 -0800 Subject: [PATCH 25/31] Fixing tests --- src/coreclr/jit/gentree.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index b8e4958cfd7a7..08988aac9b776 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8360,9 +8360,14 @@ inline bool GenTree::IsIntegralConst() const // user code. inline bool GenTree::IsIntegralConstAbsPow2() const { - if (IsIntegralConst()) + if ((gtOper == GT_CNS_INT) && isPow2(abs(AsIntConCommon()->IconValue()))) { - return isPow2(abs(AsIntConCommon()->IntegralValue())); + return true; + } + + if ((gtOper == GT_CNS_LNG) && isPow2(abs(AsIntConCommon()->LngValue()))) + { + return true; } return false; From 4c36be71b535e7e55f3af0d42b0fd1210682fdf7 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 11 Mar 2022 14:11:35 -0800 Subject: [PATCH 26/31] Fixing tests --- src/coreclr/jit/gentree.h | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 08988aac9b776..f5267d06f00a3 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8360,14 +8360,11 @@ inline bool GenTree::IsIntegralConst() const // user code. inline bool GenTree::IsIntegralConstAbsPow2() const { - if ((gtOper == GT_CNS_INT) && isPow2(abs(AsIntConCommon()->IconValue()))) + if (IsIntegralConst()) { - return true; - } - - if ((gtOper == GT_CNS_LNG) && isPow2(abs(AsIntConCommon()->LngValue()))) - { - return true; + INT64 svalue = AsIntConCommon()->IntegralValue(); + size_t value = (svalue == SSIZE_T_MIN) ? static_cast(svalue) : static_cast(abs(svalue)); + return isPow2(value); } return false; From 19f16b0a20479d3f4247b968fe4b87d4b2cc483c Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 11 Mar 2022 14:12:02 -0800 Subject: [PATCH 27/31] Fixing tests --- src/coreclr/jit/gentree.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index f5267d06f00a3..8bc6f4b59bff6 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8363,7 +8363,7 @@ inline bool GenTree::IsIntegralConstAbsPow2() const if (IsIntegralConst()) { INT64 svalue = AsIntConCommon()->IntegralValue(); - size_t value = (svalue == SSIZE_T_MIN) ? static_cast(svalue) : static_cast(abs(svalue)); + size_t value = (svalue == SSIZE_T_MIN) ? static_cast(svalue) : static_cast(abs(svalue)); return isPow2(value); } From f8921b964d071ddec71c8067589ff5163f75dbee Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 11 Mar 2022 16:00:53 -0800 Subject: [PATCH 28/31] Formatting --- src/coreclr/jit/gentree.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 8bc6f4b59bff6..8a70af9d0171a 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -8362,8 +8362,8 @@ inline bool GenTree::IsIntegralConstAbsPow2() const { if (IsIntegralConst()) { - INT64 svalue = AsIntConCommon()->IntegralValue(); - size_t value = (svalue == SSIZE_T_MIN) ? static_cast(svalue) : static_cast(abs(svalue)); + INT64 svalue = AsIntConCommon()->IntegralValue(); + size_t value = (svalue == SSIZE_T_MIN) ? static_cast(svalue) : static_cast(abs(svalue)); return isPow2(value); } From ade331d6c9d4c6599cc94247bdc65ad5d1903fe0 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 11 Mar 2022 16:09:31 -0800 Subject: [PATCH 29/31] Fixing tests --- src/coreclr/jit/gentree.h | 30 +++++++++++++++++++++++++----- src/coreclr/jit/morph.cpp | 2 +- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 8a70af9d0171a..d215b4eee7678 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -2110,6 +2110,8 @@ struct GenTree inline bool IsIntegralConst() const; + inline bool IsIntegralConstUnsignedPow2() const; + inline bool IsIntegralConstAbsPow2() const; inline bool IsIntCnsFitsInI32(); // Constant fits in INT32 @@ -8346,18 +8348,36 @@ inline bool GenTree::IsIntegralConst() const } //------------------------------------------------------------------------- -// IsIntegralConstAbsPow2: Determines whether the absolute value of -// an integral constant is the power of 2. +// IsIntegralConstUnsignedPow2: Determines whether the unsigned value of +// an integral constant is the power of 2. // // Return Value: -// Returns true if the absolute value of a GenTree's integral constant +// Returns true if the unsigned value of a GenTree's integral constant // is the power of 2. // // Notes: // Integral constant nodes store its value in signed form. -// Because this function takes the absolute value of the constant's value, -// this should handle cases where an unsigned-int was logically used in +// This should handle cases where an unsigned-int was logically used in // user code. +// +inline bool GenTree::IsIntegralConstUnsignedPow2() const +{ + if (IsIntegralConst()) + { + return isPow2((UINT64)AsIntConCommon()->IntegralValue()); + } + + return false; +} + +//------------------------------------------------------------------------- +// IsIntegralConstAbsPow2: Determines whether the absolute value of +// an integral constant is the power of 2. +// +// Return Value: +// Returns true if the absolute value of a GenTree's integral constant +// is the power of 2. +// inline bool GenTree::IsIntegralConstAbsPow2() const { if (IsIntegralConst()) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 853cc02b11afe..c4813cae00497 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11399,7 +11399,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (!optValnumCSE_phase) { #ifdef TARGET_ARM64 - if (tree->OperIs(GT_UMOD) && op2->IsIntegralConstAbsPow2()) + if (tree->OperIs(GT_UMOD) && op2->IsIntegralConstUnsignedPow2()) { // Transformation: a % b = a & (b - 1); tree = fgMorphUModToAndSub(tree->AsOp()); From 06d1124752ff320f377c157de47834878e30bbb4 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 12 Mar 2022 17:01:23 -0800 Subject: [PATCH 30/31] Feedback --- src/coreclr/jit/morph.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index c4813cae00497..16840b9905137 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14734,18 +14734,19 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) JITDUMP("\nMorphing UMOD [%06u] to And/Sub\n", dspTreeID(tree)); assert(tree->OperIs(GT_UMOD)); - assert(tree->gtOp2->IsIntegralConstAbsPow2()); + assert(tree->gtOp2->IsIntegralConstUnsignedPow2()); - var_types type = tree->TypeGet(); + const var_types type = tree->TypeGet(); - GenTree* const sub = gtNewOperNode(GT_SUB, type, tree->gtOp2, gtNewOneConNode(type)); - GenTree* const andSub = gtNewOperNode(GT_AND, type, tree->gtOp1, sub); + const size_t cnsValue = ((UINT64)tree->gtOp2->AsIntConCommon()->IntegralValue()) - 1; + GenTree* const and = gtNewOperNode(GT_AND, type, tree->gtOp1, gtNewIconNode(cnsValue, type)); - INDEBUG(andSub->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); + INDEBUG(and->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); + DEBUG_DESTROY_NODE(tree->gtOp2); DEBUG_DESTROY_NODE(tree); - return andSub; + return and; } //------------------------------------------------------------------------------ From f142ca20fc2bb9c3d6ecdef7e8ea85e62ee48300 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 12 Mar 2022 22:16:32 -0800 Subject: [PATCH 31/31] Fixing build --- src/coreclr/jit/morph.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 16840b9905137..cfbfa675991c4 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14738,15 +14738,15 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree) const var_types type = tree->TypeGet(); - const size_t cnsValue = ((UINT64)tree->gtOp2->AsIntConCommon()->IntegralValue()) - 1; - GenTree* const and = gtNewOperNode(GT_AND, type, tree->gtOp1, gtNewIconNode(cnsValue, type)); + const size_t cnsValue = (static_cast(tree->gtOp2->AsIntConCommon()->IntegralValue())) - 1; + GenTree* const newTree = gtNewOperNode(GT_AND, type, tree->gtOp1, gtNewIconNode(cnsValue, type)); - INDEBUG(and->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); + INDEBUG(newTree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); DEBUG_DESTROY_NODE(tree->gtOp2); DEBUG_DESTROY_NODE(tree); - return and; + return newTree; } //------------------------------------------------------------------------------