From 252c80a4f4bf03c2cbe879b1225fcf75f99caed9 Mon Sep 17 00:00:00 2001 From: TIHan Date: Mon, 10 Apr 2023 19:02:12 -0700 Subject: [PATCH 1/8] Combine compare and shift ops into a single compare op --- src/coreclr/jit/codegen.h | 1 + src/coreclr/jit/codegenarm64.cpp | 60 +++++++++++++++++++------------- src/coreclr/jit/lowerarmarch.cpp | 20 +++++++++++ 3 files changed, 56 insertions(+), 25 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index f86d7db1e6b305..cefb5f5c2052e5 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1552,6 +1552,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX #if defined(TARGET_ARM64) static insCond JumpKindToInsCond(emitJumpKind condition); + static insOpts ShiftOpToInsOpts(genTreeOps op); #elif defined(TARGET_XARCH) static instruction JumpKindToCmov(emitJumpKind condition); #endif diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index 6b1a20565dec2c..d47b1c0a043498 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -2626,31 +2626,7 @@ void CodeGen::genCodeForBinary(GenTreeOp* tree) } } - switch (op2->gtOper) - { - case GT_LSH: - { - opt = INS_OPTS_LSL; - break; - } - - case GT_RSH: - { - opt = INS_OPTS_ASR; - break; - } - - case GT_RSZ: - { - opt = INS_OPTS_LSR; - break; - } - - default: - { - unreached(); - } - } + opt = ShiftOpToInsOpts(op2->gtOper); emit->emitIns_R_R_R_I(ins, emitActualTypeSize(tree), targetReg, a->GetRegNum(), b->GetRegNum(), c->AsIntConCommon()->IconValue(), opt); @@ -4546,6 +4522,15 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) emit->emitIns_R_I(ins, cmpSize, op1Reg, intConst->IconValue()); } + else if (op2->isContained()) + { + assert(op2->OperIs(GT_LSH, GT_RSH, GT_RSZ)); + assert(op2->gtGetOp2()->IsCnsIntOrI()); + assert(op2->gtGetOp2()->isContained()); + + emit->emitIns_R_R_I(ins, cmpSize, op1->GetRegNum(), op2->gtGetOp1()->GetRegNum(), + op2->gtGetOp2()->AsIntConCommon()->IntegralValue(), ShiftOpToInsOpts(op2->gtOper)); + } else { emit->emitIns_R_R(ins, cmpSize, op1->GetRegNum(), op2->GetRegNum()); @@ -10329,4 +10314,29 @@ insCond CodeGen::JumpKindToInsCond(emitJumpKind condition) } } +//------------------------------------------------------------------------ +// ShiftOpToInsOpts: Convert a shift-op to a insOpts. +// +// Arguments: +// shiftOp - the shift-op tree. +// +insOpts CodeGen::ShiftOpToInsOpts(genTreeOps op) +{ + switch (op) + { + case GT_LSH: + return INS_OPTS_LSL; + + case GT_RSH: + return INS_OPTS_ASR; + + case GT_RSZ: + return INS_OPTS_LSR; + + default: + NO_WAY("expected a shift-op"); + return INS_OPTS_NONE; + } +} + #endif // TARGET_ARM64 diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 0a9f57f5469264..115184a3e52170 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -2265,6 +2265,26 @@ void Lowering::ContainCheckCast(GenTreeCast* node) void Lowering::ContainCheckCompare(GenTreeOp* cmp) { CheckImmedAndMakeContained(cmp, cmp->gtOp2); + +#ifdef TARGET_ARM64 + if (comp->opts.OptimizationEnabled() && cmp->gtGetOp2()->OperIs(GT_LSH, GT_RSH, GT_RSZ)) + { + auto isValidImmForShift = [](ssize_t imm, var_types type) + { return emitter::isValidImmShift(imm, EA_ATTR(genTypeSize(type))); }; + + GenTree* op2 = cmp->gtGetOp2(); + + if (op2->gtGetOp2()->IsCnsIntOrI() && + isValidImmForShift(op2->gtGetOp2()->AsIntConCommon()->IntegralValue(), cmp->TypeGet())) + { + op2->ClearContained(); + op2->gtGetOp1()->ClearContained(); + + MakeSrcContained(op2, op2->gtGetOp2()); + MakeSrcContained(cmp, op2); + } + } +#endif // TARGET_ARM64 } #ifdef TARGET_ARM64 From 6455122d33d02caaa902791c7c8ba195e5cc86fb Mon Sep 17 00:00:00 2001 From: TIHan Date: Mon, 10 Apr 2023 19:09:38 -0700 Subject: [PATCH 2/8] Fix comment --- src/coreclr/jit/codegenarm64.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index d47b1c0a043498..3bfe03b88ba8f3 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -10318,11 +10318,11 @@ insCond CodeGen::JumpKindToInsCond(emitJumpKind condition) // ShiftOpToInsOpts: Convert a shift-op to a insOpts. // // Arguments: -// shiftOp - the shift-op tree. +// shiftOp - the shift-op. // -insOpts CodeGen::ShiftOpToInsOpts(genTreeOps op) +insOpts CodeGen::ShiftOpToInsOpts(genTreeOps shiftOp) { - switch (op) + switch (shiftOp) { case GT_LSH: return INS_OPTS_LSL; From cb932ba1abf6bb9e5a810b6c178179635aa69256 Mon Sep 17 00:00:00 2001 From: TIHan Date: Mon, 10 Apr 2023 20:10:00 -0700 Subject: [PATCH 3/8] Expanding IsContainableBinaryOp. --- src/coreclr/jit/lowerarmarch.cpp | 68 +++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 115184a3e52170..58f5364fe89e12 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -269,7 +269,7 @@ bool Lowering::IsContainableBinaryOp(GenTree* parentNode, GenTree* childNode) co return false; } - if (parentNode->OperIs(GT_CMP, GT_OR, GT_XOR)) + if (parentNode->OperIs(GT_CMP, GT_OR, GT_XOR) || parentNode->OperIsCompare()) { if (IsInvariantInRange(childNode, parentNode)) { @@ -2264,27 +2264,67 @@ void Lowering::ContainCheckCast(GenTreeCast* node) // void Lowering::ContainCheckCompare(GenTreeOp* cmp) { - CheckImmedAndMakeContained(cmp, cmp->gtOp2); + GenTree* op1 = cmp->gtGetOp1(); + GenTree* op2 = cmp->gtGetOp2(); + + if (CheckImmedAndMakeContained(cmp, op2)) + return; #ifdef TARGET_ARM64 - if (comp->opts.OptimizationEnabled() && cmp->gtGetOp2()->OperIs(GT_LSH, GT_RSH, GT_RSZ)) + if (comp->opts.OptimizationEnabled()) { - auto isValidImmForShift = [](ssize_t imm, var_types type) - { return emitter::isValidImmShift(imm, EA_ATTR(genTypeSize(type))); }; - - GenTree* op2 = cmp->gtGetOp2(); + if (IsContainableBinaryOp(cmp, op2)) + { + MakeSrcContained(cmp, op2); + return; + } - if (op2->gtGetOp2()->IsCnsIntOrI() && - isValidImmForShift(op2->gtGetOp2()->AsIntConCommon()->IntegralValue(), cmp->TypeGet())) + if (cmp->OperIs(GT_EQ, GT_NE) && IsContainableBinaryOp(cmp, op1)) { - op2->ClearContained(); - op2->gtGetOp1()->ClearContained(); + MakeSrcContained(cmp, op1); - MakeSrcContained(op2, op2->gtGetOp2()); - MakeSrcContained(cmp, op2); + std::swap(cmp->gtOp1, cmp->gtOp2); + return; } } -#endif // TARGET_ARM64 +#endif + +//#ifdef TARGET_ARM64 +// if (comp->opts.OptimizationEnabled() && cmp->gtGetOp2()->OperIs(GT_LSH, GT_RSH, GT_RSZ)) +// { +// auto isValidImmForShift = [](ssize_t imm, var_types type) +// { return emitter::isValidImmShift(imm, EA_ATTR(genTypeSize(type))); }; +// +// GenTree* op2 = cmp->gtGetOp2(); +// +// if (op2->gtGetOp2()->IsCnsIntOrI() && +// isValidImmForShift(op2->gtGetOp2()->AsIntConCommon()->IntegralValue(), cmp->TypeGet()) && +// IsContainableBinaryOp(cmp, cmp->gtGetOp2())) +// { +// op2->ClearContained(); +// op2->gtGetOp1()->ClearContained(); +// +// MakeSrcContained(op2, op2->gtGetOp2()); +// MakeSrcContained(cmp, op2); +// } +// else if (cmp->OperIsCommutative() && IsContainableBinaryOp(cmp, cmp->gtGetOp1())) +// { +// } +// +// // if (node->OperIsCommutative() && IsContainableBinaryOp(node, op1)) +// //{ +// // if (op1->OperIs(GT_CAST)) +// // { +// // // We want to prefer the combined op here over containment of the cast op +// // op1->AsCast()->CastOp()->ClearContained(); +// // } +// // MakeSrcContained(node, op1); +// +// // std::swap(node->gtOp1, node->gtOp2); +// // return; +// //} +// } +//#endif // TARGET_ARM64 } #ifdef TARGET_ARM64 From 860c8dec1edd8397902b897615777d48d06066a2 Mon Sep 17 00:00:00 2001 From: TIHan Date: Mon, 10 Apr 2023 20:12:52 -0700 Subject: [PATCH 4/8] Remove commented code --- src/coreclr/jit/lowerarmarch.cpp | 37 -------------------------------- 1 file changed, 37 deletions(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 58f5364fe89e12..6429a6be6f18cb 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -2288,43 +2288,6 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) } } #endif - -//#ifdef TARGET_ARM64 -// if (comp->opts.OptimizationEnabled() && cmp->gtGetOp2()->OperIs(GT_LSH, GT_RSH, GT_RSZ)) -// { -// auto isValidImmForShift = [](ssize_t imm, var_types type) -// { return emitter::isValidImmShift(imm, EA_ATTR(genTypeSize(type))); }; -// -// GenTree* op2 = cmp->gtGetOp2(); -// -// if (op2->gtGetOp2()->IsCnsIntOrI() && -// isValidImmForShift(op2->gtGetOp2()->AsIntConCommon()->IntegralValue(), cmp->TypeGet()) && -// IsContainableBinaryOp(cmp, cmp->gtGetOp2())) -// { -// op2->ClearContained(); -// op2->gtGetOp1()->ClearContained(); -// -// MakeSrcContained(op2, op2->gtGetOp2()); -// MakeSrcContained(cmp, op2); -// } -// else if (cmp->OperIsCommutative() && IsContainableBinaryOp(cmp, cmp->gtGetOp1())) -// { -// } -// -// // if (node->OperIsCommutative() && IsContainableBinaryOp(node, op1)) -// //{ -// // if (op1->OperIs(GT_CAST)) -// // { -// // // We want to prefer the combined op here over containment of the cast op -// // op1->AsCast()->CastOp()->ClearContained(); -// // } -// // MakeSrcContained(node, op1); -// -// // std::swap(node->gtOp1, node->gtOp2); -// // return; -// //} -// } -//#endif // TARGET_ARM64 } #ifdef TARGET_ARM64 From 65592017952c7b29403172b7b6092e37243b9a9a Mon Sep 17 00:00:00 2001 From: TIHan Date: Tue, 11 Apr 2023 13:38:56 -0700 Subject: [PATCH 5/8] Added more cases --- src/coreclr/jit/lowerarmarch.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 6429a6be6f18cb..2b718dcfdde3a1 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -2271,7 +2271,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) return; #ifdef TARGET_ARM64 - if (comp->opts.OptimizationEnabled()) + if (comp->opts.OptimizationEnabled() && cmp->OperIsCompare()) { if (IsContainableBinaryOp(cmp, op2)) { @@ -2279,11 +2279,11 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) return; } - if (cmp->OperIs(GT_EQ, GT_NE) && IsContainableBinaryOp(cmp, op1)) + if (IsContainableBinaryOp(cmp, op1)) { MakeSrcContained(cmp, op1); - std::swap(cmp->gtOp1, cmp->gtOp2); + cmp->ChangeOper(cmp->SwapRelop(cmp->gtOper)); return; } } From b350e979be4d462a410ee3a27a013a4d042cdd71 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Wed, 12 Apr 2023 17:23:49 -0700 Subject: [PATCH 6/8] Update codegenarm64.cpp --- src/coreclr/jit/codegenarm64.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index 3bfe03b88ba8f3..e8c067ac07ccff 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -10318,7 +10318,7 @@ insCond CodeGen::JumpKindToInsCond(emitJumpKind condition) // ShiftOpToInsOpts: Convert a shift-op to a insOpts. // // Arguments: -// shiftOp - the shift-op. +// shiftOp - the shift-op // insOpts CodeGen::ShiftOpToInsOpts(genTreeOps shiftOp) { From c197cd864878614d36eef0f10dab4c7195e56caf Mon Sep 17 00:00:00 2001 From: TIHan Date: Thu, 13 Apr 2023 13:15:42 -0700 Subject: [PATCH 7/8] Feedback --- src/coreclr/jit/lowerarmarch.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 2b718dcfdde3a1..0d20058c0cb2ba 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -2270,6 +2270,13 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) if (CheckImmedAndMakeContained(cmp, op2)) return; + if (CheckImmedAndMakeContained(cmp, op1)) + { + std::swap(cmp->gtOp1, cmp->gtOp2); + cmp->ChangeOper(cmp->SwapRelop(cmp->gtOper)); + return; + } + #ifdef TARGET_ARM64 if (comp->opts.OptimizationEnabled() && cmp->OperIsCompare()) { From d0851d18b6dc8f976bbbf04b56ccf791d4f11c49 Mon Sep 17 00:00:00 2001 From: TIHan Date: Thu, 13 Apr 2023 13:16:18 -0700 Subject: [PATCH 8/8] Feedback --- src/coreclr/jit/lowerarmarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 0d20058c0cb2ba..8ebb2c044a7f7c 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -2270,7 +2270,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) if (CheckImmedAndMakeContained(cmp, op2)) return; - if (CheckImmedAndMakeContained(cmp, op1)) + if (cmp->OperIsCompare() && CheckImmedAndMakeContained(cmp, op1)) { std::swap(cmp->gtOp1, cmp->gtOp2); cmp->ChangeOper(cmp->SwapRelop(cmp->gtOper));