From 14b52a85ce2baef9c7cd44848715ed7600544aaf Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Fri, 14 Aug 2020 11:47:20 -0700 Subject: [PATCH 1/5] Simple arithmetic optimization with GT_NEG --- src/coreclr/src/jit/morph.cpp | 188 ++++++++++++++++++++++++++-------- 1 file changed, 145 insertions(+), 43 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 08b7bf0368555a..bb365b7f693798 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -13324,6 +13324,61 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) /* No match - exit */ } + + if (opts.OptimizationEnabled() && + (((op1->gtFlags & GTF_EXCEPT) == 0) || ((op2->gtFlags & GTF_EXCEPT) == 0))) + { + bool op1OperIsGT_NEG = op1->OperIs(GT_NEG); + bool op2OperIsGT_NEG = op2->OperIs(GT_NEG); + + // a - -b = > a + b + // SUB(a, (NEG(b)) => ADD(a, b) + + if (!op1OperIsGT_NEG && op2OperIsGT_NEG) + { + // tree: SUB + // op1: a + // op2: NEG + // op2Child: b + + GenTree* op2Child = op2->AsOp()->gtOp1; // b + oper = GT_ADD; + tree->SetOper(oper); + tree->AsOp()->gtOp1 = op1; // no change + tree->AsOp()->gtOp2 = op2Child; + + DEBUG_DESTROY_NODE(op2); + + // op1 = op1; // no change + op2 = op2Child; + } + + // -a - -b = > b - a + // SUB(NEG(a), (NEG(b)) => SUB(b, a) + + if (op1OperIsGT_NEG && op2OperIsGT_NEG) + { + // tree: SUB + // op1: NEG + // op1Child: a + // op2: NEG + // op2Child: b + + GenTree* op1Child = op1->AsOp()->gtOp1; // a + GenTree* op2Child = op2->AsOp()->gtOp1; // b + oper = GT_SUB; // no change + tree->SetOper(oper); // no change + tree->AsOp()->gtOp1 = op2Child; + tree->AsOp()->gtOp2 = op1Child; + + DEBUG_DESTROY_NODE(op1); + DEBUG_DESTROY_NODE(op2); + + op1 = op2Child; + op2 = op1Child; + } + } + break; #ifdef TARGET_ARM64 @@ -13533,6 +13588,57 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) } } } + + if (opts.OptimizationEnabled() && + (((op1->gtFlags & GTF_EXCEPT) == 0) || ((op2->gtFlags & GTF_EXCEPT) == 0))) + { + bool op1OperIsGT_NEG = op1->OperIs(GT_NEG); + bool op2OperIsGT_NEG = op2->OperIs(GT_NEG); + + // - a + b = > b - a + // ADD((NEG(a), b) => SUB(b, a) + + if (op1OperIsGT_NEG && !op2OperIsGT_NEG) + { + // tree: ADD + // op1: NEG + // op2: b + // op1Child: a + + GenTree* op1Child = op1->AsOp()->gtOp1; // a + oper = GT_SUB; + tree->SetOper(oper); + tree->AsOp()->gtOp1 = op2; + tree->AsOp()->gtOp2 = op1Child; + + DEBUG_DESTROY_NODE(op1); + + op1 = op2; + op2 = op1Child; + } + + // a + -b = > a - b + // ADD(a, (NEG(b)) => SUB(a, b) + + if (!op1OperIsGT_NEG && op2OperIsGT_NEG) + { + // tree: ADD + // op1: a + // op2: NEG + // op2Child: b + + GenTree* op2Child = op2->AsOp()->gtOp1; // a + oper = GT_SUB; + tree->SetOper(oper); + // tree->AsOp()->gtOp1 = op1; // no change + tree->AsOp()->gtOp2 = op2Child; + + DEBUG_DESTROY_NODE(op2); + + // op1 = op2; + op2 = op2Child; + } + } } /* See if we can fold GT_MUL by const nodes */ else if (oper == GT_MUL && op2->IsCnsIntOrI() && !optValnumCSE_phase) @@ -14957,23 +15063,22 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) // / \ / \. // x AND x AND // / \ / \. - // y 31 ADD 31 + // y 31 SUB 31 // / \. - // NEG 32 - // | - // y + // 32 y + // The patterns recognized: - // (x << (y & M)) op (x >>> ((-y + N) & M)) - // (x >>> ((-y + N) & M)) op (x << (y & M)) + // (x << (y & M)) op (x >>> ((N - y) & M)) + // (x >>> ((N - y) & M)) op (x << (y & M)) // - // (x << y) op (x >>> (-y + N)) - // (x >> > (-y + N)) op (x << y) + // (x << y) op (x >>> (N - y)) + // (x >> > (N - y)) op (x << y) // - // (x >>> (y & M)) op (x << ((-y + N) & M)) - // (x << ((-y + N) & M)) op (x >>> (y & M)) + // (x >>> (y & M)) op (x << ((N - y) & M)) + // (x << ((N - y) & M)) op (x >>> (y & M)) // - // (x >>> y) op (x << (-y + N)) - // (x << (-y + N)) op (x >>> y) + // (x >>> y) op (x << (N - y)) + // (x << (N - y)) op (x >>> y) // // (x << c1) op (x >>> c2) // (x >>> c1) op (x << c2) @@ -15072,55 +15177,52 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) return tree; } - GenTree* shiftIndexWithAdd = nullptr; - GenTree* shiftIndexWithoutAdd = nullptr; + GenTree* shiftIndexWithSUB = nullptr; + GenTree* shiftIndexWithoutSUB = nullptr; genTreeOps rotateOp = GT_NONE; GenTree* rotateIndex = nullptr; - if (leftShiftIndex->OperGet() == GT_ADD) + if (leftShiftIndex->OperGet() == GT_SUB) { - shiftIndexWithAdd = leftShiftIndex; - shiftIndexWithoutAdd = rightShiftIndex; + shiftIndexWithSUB = leftShiftIndex; + shiftIndexWithoutSUB = rightShiftIndex; rotateOp = GT_ROR; } - else if (rightShiftIndex->OperGet() == GT_ADD) + else if (rightShiftIndex->OperGet() == GT_SUB) { - shiftIndexWithAdd = rightShiftIndex; - shiftIndexWithoutAdd = leftShiftIndex; + shiftIndexWithSUB = rightShiftIndex; + shiftIndexWithoutSUB = leftShiftIndex; rotateOp = GT_ROL; } - if (shiftIndexWithAdd != nullptr) + if (shiftIndexWithSUB != nullptr) { - if (shiftIndexWithAdd->gtGetOp2()->IsCnsIntOrI()) + if (shiftIndexWithSUB->gtGetOp1()->IsCnsIntOrI()) { - if (shiftIndexWithAdd->gtGetOp2()->AsIntCon()->gtIconVal == rotatedValueBitSize) + if (shiftIndexWithSUB->gtGetOp1()->AsIntCon()->gtIconVal == rotatedValueBitSize) { - if (shiftIndexWithAdd->gtGetOp1()->OperGet() == GT_NEG) + if (GenTree::Compare(shiftIndexWithSUB->gtGetOp2(), shiftIndexWithoutSUB)) { - if (GenTree::Compare(shiftIndexWithAdd->gtGetOp1()->gtGetOp1(), shiftIndexWithoutAdd)) - { - // We found one of these patterns: - // (x << (y & M)) | (x >>> ((-y + N) & M)) - // (x << y) | (x >>> (-y + N)) - // (x >>> (y & M)) | (x << ((-y + N) & M)) - // (x >>> y) | (x << (-y + N)) - // where N == bitsize(x), M is const, and - // M & (N - 1) == N - 1 - CLANG_FORMAT_COMMENT_ANCHOR; + // We found one of these patterns: + // (x << (y & M)) | (x >>> ((N - y) & M)) + // (x << y) | (x >>> (N - y)) + // (x >>> (y & M)) | (x << ((N - y) & M)) + // (x >>> y) | (x << (N - y)) + // where N == bitsize(x), M is const, and + // M & (N - 1) == N - 1 + CLANG_FORMAT_COMMENT_ANCHOR; #ifndef TARGET_64BIT - if (!shiftIndexWithoutAdd->IsCnsIntOrI() && (rotatedValueBitSize == 64)) - { - // TODO-X86-CQ: we need to handle variable-sized long shifts specially on x86. - // GT_LSH, GT_RSH, and GT_RSZ have helpers for this case. We may need - // to add helpers for GT_ROL and GT_ROR. - return tree; - } + if (!shiftIndexWithoutSUB->IsCnsIntOrI() && (rotatedValueBitSize == 64)) + { + // TODO-X86-CQ: we need to handle variable-sized long shifts specially on x86. + // GT_LSH, GT_RSH, and GT_RSZ have helpers for this case. We may need + // to add helpers for GT_ROL and GT_ROR. + return tree; + } #endif - rotateIndex = shiftIndexWithoutAdd; - } + rotateIndex = shiftIndexWithoutSUB; } } } From ffebda95280288bb9d3e1c6cd5326daad6c27fc7 Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Fri, 21 Aug 2020 21:32:13 -0700 Subject: [PATCH 2/5] Skip GT_NEG optimization when an operand is constant. Revert bitwise rotation pattern --- src/coreclr/src/jit/morph.cpp | 103 ++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 49 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index bb365b7f693798..f3f1c7faf160aa 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -13325,16 +13325,15 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) /* No match - exit */ } + // Skip optimization if non-NEG operand is constant. + // Both op1 and op2 are not constant because it was already checked above. if (opts.OptimizationEnabled() && (((op1->gtFlags & GTF_EXCEPT) == 0) || ((op2->gtFlags & GTF_EXCEPT) == 0))) { - bool op1OperIsGT_NEG = op1->OperIs(GT_NEG); - bool op2OperIsGT_NEG = op2->OperIs(GT_NEG); - // a - -b = > a + b // SUB(a, (NEG(b)) => ADD(a, b) - if (!op1OperIsGT_NEG && op2OperIsGT_NEG) + if (!op1->OperIs(GT_NEG) && op2->OperIs(GT_NEG)) { // tree: SUB // op1: a @@ -13356,7 +13355,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // -a - -b = > b - a // SUB(NEG(a), (NEG(b)) => SUB(b, a) - if (op1OperIsGT_NEG && op2OperIsGT_NEG) + if (op1->OperIs(GT_NEG) && op2->OperIs(GT_NEG)) { // tree: SUB // op1: NEG @@ -13592,13 +13591,12 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (opts.OptimizationEnabled() && (((op1->gtFlags & GTF_EXCEPT) == 0) || ((op2->gtFlags & GTF_EXCEPT) == 0))) { - bool op1OperIsGT_NEG = op1->OperIs(GT_NEG); - bool op2OperIsGT_NEG = op2->OperIs(GT_NEG); - // - a + b = > b - a // ADD((NEG(a), b) => SUB(b, a) - if (op1OperIsGT_NEG && !op2OperIsGT_NEG) + // Skip optimization if non-NEG operand is constant. + if (op1->OperIs(GT_NEG) && !op2->OperIs(GT_NEG) && + !(op2->IsCnsIntOrI() && varTypeIsIntegralOrI(typ))) { // tree: ADD // op1: NEG @@ -13620,8 +13618,11 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // a + -b = > a - b // ADD(a, (NEG(b)) => SUB(a, b) - if (!op1OperIsGT_NEG && op2OperIsGT_NEG) + if (!op1->OperIs(GT_NEG) && op2->OperIs(GT_NEG)) { + // a is non cosntant because it was already canonicalized to have + // variable on the left and constant on the right. + // tree: ADD // op1: a // op2: NEG @@ -15063,22 +15064,23 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) // / \ / \. // x AND x AND // / \ / \. - // y 31 SUB 31 + // y 31 ADD 31 // / \. - // 32 y - + // NEG 32 + // | + // y // The patterns recognized: - // (x << (y & M)) op (x >>> ((N - y) & M)) - // (x >>> ((N - y) & M)) op (x << (y & M)) + // (x << (y & M)) op (x >>> ((-y + N) & M)) + // (x >>> ((-y + N) & M)) op (x << (y & M)) // - // (x << y) op (x >>> (N - y)) - // (x >> > (N - y)) op (x << y) + // (x << y) op (x >>> (-y + N)) + // (x >> > (-y + N)) op (x << y) // - // (x >>> (y & M)) op (x << ((N - y) & M)) - // (x << ((N - y) & M)) op (x >>> (y & M)) + // (x >>> (y & M)) op (x << ((-y + N) & M)) + // (x << ((-y + N) & M)) op (x >>> (y & M)) // - // (x >>> y) op (x << (N - y)) - // (x << (N - y)) op (x >>> y) + // (x >>> y) op (x << (-y + N)) + // (x << (-y + N)) op (x >>> y) // // (x << c1) op (x >>> c2) // (x >>> c1) op (x << c2) @@ -15177,52 +15179,55 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) return tree; } - GenTree* shiftIndexWithSUB = nullptr; - GenTree* shiftIndexWithoutSUB = nullptr; + GenTree* shiftIndexWithAdd = nullptr; + GenTree* shiftIndexWithoutAdd = nullptr; genTreeOps rotateOp = GT_NONE; GenTree* rotateIndex = nullptr; - if (leftShiftIndex->OperGet() == GT_SUB) + if (leftShiftIndex->OperGet() == GT_ADD) { - shiftIndexWithSUB = leftShiftIndex; - shiftIndexWithoutSUB = rightShiftIndex; + shiftIndexWithAdd = leftShiftIndex; + shiftIndexWithoutAdd = rightShiftIndex; rotateOp = GT_ROR; } - else if (rightShiftIndex->OperGet() == GT_SUB) + else if (rightShiftIndex->OperGet() == GT_ADD) { - shiftIndexWithSUB = rightShiftIndex; - shiftIndexWithoutSUB = leftShiftIndex; + shiftIndexWithAdd = rightShiftIndex; + shiftIndexWithoutAdd = leftShiftIndex; rotateOp = GT_ROL; } - if (shiftIndexWithSUB != nullptr) + if (shiftIndexWithAdd != nullptr) { - if (shiftIndexWithSUB->gtGetOp1()->IsCnsIntOrI()) + if (shiftIndexWithAdd->gtGetOp2()->IsCnsIntOrI()) { - if (shiftIndexWithSUB->gtGetOp1()->AsIntCon()->gtIconVal == rotatedValueBitSize) + if (shiftIndexWithAdd->gtGetOp2()->AsIntCon()->gtIconVal == rotatedValueBitSize) { - if (GenTree::Compare(shiftIndexWithSUB->gtGetOp2(), shiftIndexWithoutSUB)) + if (shiftIndexWithAdd->gtGetOp1()->OperGet() == GT_NEG) { - // We found one of these patterns: - // (x << (y & M)) | (x >>> ((N - y) & M)) - // (x << y) | (x >>> (N - y)) - // (x >>> (y & M)) | (x << ((N - y) & M)) - // (x >>> y) | (x << (N - y)) - // where N == bitsize(x), M is const, and - // M & (N - 1) == N - 1 - CLANG_FORMAT_COMMENT_ANCHOR; + if (GenTree::Compare(shiftIndexWithAdd->gtGetOp1()->gtGetOp1(), shiftIndexWithoutAdd)) + { + // We found one of these patterns: + // (x << (y & M)) | (x >>> ((-y + N) & M)) + // (x << y) | (x >>> (-y + N)) + // (x >>> (y & M)) | (x << ((-y + N) & M)) + // (x >>> y) | (x << (-y + N)) + // where N == bitsize(x), M is const, and + // M & (N - 1) == N - 1 + CLANG_FORMAT_COMMENT_ANCHOR; #ifndef TARGET_64BIT - if (!shiftIndexWithoutSUB->IsCnsIntOrI() && (rotatedValueBitSize == 64)) - { - // TODO-X86-CQ: we need to handle variable-sized long shifts specially on x86. - // GT_LSH, GT_RSH, and GT_RSZ have helpers for this case. We may need - // to add helpers for GT_ROL and GT_ROR. - return tree; - } + if (!shiftIndexWithoutAdd->IsCnsIntOrI() && (rotatedValueBitSize == 64)) + { + // TODO-X86-CQ: we need to handle variable-sized long shifts specially on x86. + // GT_LSH, GT_RSH, and GT_RSZ have helpers for this case. We may need + // to add helpers for GT_ROL and GT_ROR. + return tree; + } #endif - rotateIndex = shiftIndexWithoutSUB; + rotateIndex = shiftIndexWithoutAdd; + } } } } From a2613a04fae917b44d89b98fede7cf31af74e101 Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Mon, 2 Nov 2020 16:49:34 -0800 Subject: [PATCH 3/5] Fixed Value Numbering assert --- src/coreclr/src/jit/morph.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index f3f1c7faf160aa..086fd173c0fbd9 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -13327,7 +13327,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // Skip optimization if non-NEG operand is constant. // Both op1 and op2 are not constant because it was already checked above. - if (opts.OptimizationEnabled() && + if (opts.OptimizationEnabled() && fgGlobalMorph && (((op1->gtFlags & GTF_EXCEPT) == 0) || ((op2->gtFlags & GTF_EXCEPT) == 0))) { // a - -b = > a + b @@ -13342,7 +13342,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) GenTree* op2Child = op2->AsOp()->gtOp1; // b oper = GT_ADD; - tree->SetOper(oper); + tree->SetOper(oper, GenTree::PRESERVE_VN); tree->AsOp()->gtOp1 = op1; // no change tree->AsOp()->gtOp2 = op2Child; @@ -13365,8 +13365,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) GenTree* op1Child = op1->AsOp()->gtOp1; // a GenTree* op2Child = op2->AsOp()->gtOp1; // b - oper = GT_SUB; // no change - tree->SetOper(oper); // no change + // oper = GT_SUB; // no change + // tree->SetOper(oper, GenTree::PRESERVE_VN); // no change tree->AsOp()->gtOp1 = op2Child; tree->AsOp()->gtOp2 = op1Child; @@ -13588,7 +13588,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) } } - if (opts.OptimizationEnabled() && + if (opts.OptimizationEnabled() && fgGlobalMorph && (((op1->gtFlags & GTF_EXCEPT) == 0) || ((op2->gtFlags & GTF_EXCEPT) == 0))) { // - a + b = > b - a @@ -13605,7 +13605,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) GenTree* op1Child = op1->AsOp()->gtOp1; // a oper = GT_SUB; - tree->SetOper(oper); + tree->SetOper(oper, GenTree::PRESERVE_VN); tree->AsOp()->gtOp1 = op2; tree->AsOp()->gtOp2 = op1Child; @@ -13630,7 +13630,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) GenTree* op2Child = op2->AsOp()->gtOp1; // a oper = GT_SUB; - tree->SetOper(oper); + tree->SetOper(oper, GenTree::PRESERVE_VN); // tree->AsOp()->gtOp1 = op1; // no change tree->AsOp()->gtOp2 = op2Child; From d3dc19fb37b55dadb6731f8a32f02c47373159df Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Wed, 4 Nov 2020 19:49:41 -0800 Subject: [PATCH 4/5] Cleaned up code and comments for simple GT_NEG optimization --- src/coreclr/src/jit/morph.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 086fd173c0fbd9..73b6fdcc9e52d6 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -13343,12 +13343,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) GenTree* op2Child = op2->AsOp()->gtOp1; // b oper = GT_ADD; tree->SetOper(oper, GenTree::PRESERVE_VN); - tree->AsOp()->gtOp1 = op1; // no change tree->AsOp()->gtOp2 = op2Child; DEBUG_DESTROY_NODE(op2); - // op1 = op1; // no change op2 = op2Child; } @@ -13365,8 +13363,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) GenTree* op1Child = op1->AsOp()->gtOp1; // a GenTree* op2Child = op2->AsOp()->gtOp1; // b - // oper = GT_SUB; // no change - // tree->SetOper(oper, GenTree::PRESERVE_VN); // no change tree->AsOp()->gtOp1 = op2Child; tree->AsOp()->gtOp2 = op1Child; @@ -13631,12 +13627,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) GenTree* op2Child = op2->AsOp()->gtOp1; // a oper = GT_SUB; tree->SetOper(oper, GenTree::PRESERVE_VN); - // tree->AsOp()->gtOp1 = op1; // no change tree->AsOp()->gtOp2 = op2Child; DEBUG_DESTROY_NODE(op2); - // op1 = op2; op2 = op2Child; } } From b4b7b992e72df2ddd2bca82d8c7e2ea076f82331 Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Thu, 5 Nov 2020 17:33:28 -0800 Subject: [PATCH 5/5] Formatting --- src/coreclr/src/jit/morph.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 73b6fdcc9e52d6..f695ce5a73ae08 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -13361,8 +13361,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // op2: NEG // op2Child: b - GenTree* op1Child = op1->AsOp()->gtOp1; // a - GenTree* op2Child = op2->AsOp()->gtOp1; // b + GenTree* op1Child = op1->AsOp()->gtOp1; // a + GenTree* op2Child = op2->AsOp()->gtOp1; // b tree->AsOp()->gtOp1 = op2Child; tree->AsOp()->gtOp2 = op1Child;