-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[RISCV] Use RISCVISD::SHL_ADD in transformAddShlImm #89832
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RISCV] Use RISCVISD::SHL_ADD in transformAddShlImm #89832
Conversation
Doing so avoids negative interactions with other combines which don't know the shl_add is a single instruction. From the commit log, we've had several combine loops already. This was originally posted as part of llvm#88791, where a bug was pointed out. That bug was fixed by llvm#89789 which hits the same issue from another angle. To confirm the fix, I included the reduce test case here.
@llvm/pr-subscribers-backend-risc-v Author: Philip Reames (preames) ChangesDoing so avoids negative interactions with other combines which don't know the shl_add is a single instruction. From the commit log, we've had several combine loops already. This was originally posted as part of #88791, where a bug was pointed out. That bug was fixed by #89789 which hits the same issue from another angle. To confirm the fix, I included the reduced test case here. Full diff: https://github.com/llvm/llvm-project/pull/89832.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 9c66f09a0cbc85..e6c765d5fc1d51 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -12817,10 +12817,9 @@ static SDValue transformAddShlImm(SDNode *N, SelectionDAG &DAG,
SDLoc DL(N);
SDValue NS = (C0 < C1) ? N0->getOperand(0) : N1->getOperand(0);
SDValue NL = (C0 > C1) ? N0->getOperand(0) : N1->getOperand(0);
- SDValue NA0 =
- DAG.getNode(ISD::SHL, DL, VT, NL, DAG.getConstant(Diff, DL, VT));
- SDValue NA1 = DAG.getNode(ISD::ADD, DL, VT, NA0, NS);
- return DAG.getNode(ISD::SHL, DL, VT, NA1, DAG.getConstant(Bits, DL, VT));
+ SDValue SHADD =
+ DAG.getNode(RISCVISD::SHL_ADD, DL, VT, NL, DAG.getConstant(Diff, DL, VT), NS);
+ return DAG.getNode(ISD::SHL, DL, VT, SHADD, DAG.getConstant(Bits, DL, VT));
}
// Combine a constant select operand into its use:
@@ -13056,14 +13055,17 @@ static SDValue combineAddOfBooleanXor(SDNode *N, SelectionDAG &DAG) {
N0.getOperand(0));
}
-static SDValue performADDCombine(SDNode *N, SelectionDAG &DAG,
+static SDValue performADDCombine(SDNode *N,
+ TargetLowering::DAGCombinerInfo &DCI,
const RISCVSubtarget &Subtarget) {
+ SelectionDAG &DAG = DCI.DAG;
if (SDValue V = combineAddOfBooleanXor(N, DAG))
return V;
if (SDValue V = transformAddImmMulImm(N, DAG, Subtarget))
return V;
- if (SDValue V = transformAddShlImm(N, DAG, Subtarget))
- return V;
+ if (!DCI.isBeforeLegalize() && !DCI.isCalledByLegalizer())
+ if (SDValue V = transformAddShlImm(N, DAG, Subtarget))
+ return V;
if (SDValue V = combineBinOpToReduce(N, DAG, Subtarget))
return V;
if (SDValue V = combineBinOpOfExtractToReduceTree(N, DAG, Subtarget))
@@ -16027,7 +16029,7 @@ SDValue RISCVTargetLowering::PerformDAGCombine(SDNode *N,
return V;
if (SDValue V = combineToVWMACC(N, DAG, Subtarget))
return V;
- return performADDCombine(N, DAG, Subtarget);
+ return performADDCombine(N, DCI, Subtarget);
}
case ISD::SUB: {
if (SDValue V = combineBinOp_VLToVWBinOp_VL(N, DCI, Subtarget))
diff --git a/llvm/test/CodeGen/RISCV/addimm-mulimm.ll b/llvm/test/CodeGen/RISCV/addimm-mulimm.ll
index 736c8e7d55c75b..190b38cb577355 100644
--- a/llvm/test/CodeGen/RISCV/addimm-mulimm.ll
+++ b/llvm/test/CodeGen/RISCV/addimm-mulimm.ll
@@ -898,3 +898,29 @@ define i1 @pr53831(i32 %x) {
%tmp5 = icmp eq i32 %tmp4, %tmp2
ret i1 %tmp5
}
+
+define i64 @sh2add_uw(i64 signext %0, i32 signext %1) {
+; RV32IMB-LABEL: sh2add_uw:
+; RV32IMB: # %bb.0: # %entry
+; RV32IMB-NEXT: srli a3, a2, 27
+; RV32IMB-NEXT: slli a2, a2, 5
+; RV32IMB-NEXT: srli a4, a0, 29
+; RV32IMB-NEXT: sh3add a1, a1, a4
+; RV32IMB-NEXT: sh3add a0, a0, a2
+; RV32IMB-NEXT: sltu a2, a0, a2
+; RV32IMB-NEXT: add a1, a3, a1
+; RV32IMB-NEXT: add a1, a1, a2
+; RV32IMB-NEXT: ret
+;
+; RV64IMB-LABEL: sh2add_uw:
+; RV64IMB: # %bb.0: # %entry
+; RV64IMB-NEXT: sh2add.uw a0, a1, a0
+; RV64IMB-NEXT: slli a0, a0, 3
+; RV64IMB-NEXT: ret
+entry:
+ %2 = zext i32 %1 to i64
+ %3 = shl i64 %2, 5
+ %4 = shl i64 %0, 3
+ %5 = add i64 %3, %4
+ ret i64 %5
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Doing so avoids negative interactions with other combines which don't know the shl_add is a single instruction.
Actually it works as expected. So this patch is not an NFC.
Doing so avoids negative interactions with other combines which don't know the shl_add is a single instruction. From the commit log, we've had several combine loops already.
This was originally posted as part of #88791, where a bug was pointed out. That bug was fixed by #89789 which hits the same issue from another angle. To confirm the fix, I included the reduced test case here.