Skip to content
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] Add freeze when expanding mul by constant to two or more uses #89290

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

preames
Copy link
Collaborator

@preames preames commented Apr 18, 2024

topperc pointed this out in review of #88791, but I believe the problem applies
here as well. Worth noting is that the code I introduced with this bug was mostly copied from other targets - which
also have this bug.

topperc pointed this out in review of llvm#88791, but I believe the problem applies here as well.

Worth noting is that the code I introduced with this bug was mostly
copied from other targets - which also have this bug.
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 18, 2024

@llvm/pr-subscribers-backend-risc-v

Author: Philip Reames (preames)

Changes

topperc pointed this out in review of #88791, but I believe the problem applies
here as well. Worth noting is that the code I introduced with this bug was mostly copied from other targets - which
also have this bug.


Full diff: https://github.com/llvm/llvm-project/pull/89290.diff

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+12-11)
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index b0deb1d2669952..b48033956acf3e 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -13430,10 +13430,11 @@ static SDValue expandMul(SDNode *N, SelectionDAG &DAG,
     if (ScaleShift >= 1 && ScaleShift < 4) {
       unsigned ShiftAmt = Log2_64((MulAmt & (MulAmt - 1)));
       SDLoc DL(N);
-      SDValue Shift1 = DAG.getNode(ISD::SHL, DL, VT, N->getOperand(0),
-                                   DAG.getConstant(ShiftAmt, DL, VT));
-      SDValue Shift2 = DAG.getNode(ISD::SHL, DL, VT, N->getOperand(0),
-                                   DAG.getConstant(ScaleShift, DL, VT));
+      SDValue X = DAG.getFreeze(N->getOperand(0));
+      SDValue Shift1 =
+          DAG.getNode(ISD::SHL, DL, VT, X, DAG.getConstant(ShiftAmt, DL, VT));
+      SDValue Shift2 =
+          DAG.getNode(ISD::SHL, DL, VT, X, DAG.getConstant(ScaleShift, DL, VT));
       return DAG.getNode(ISD::ADD, DL, VT, Shift1, Shift2);
     }
   }
@@ -13464,13 +13465,13 @@ static SDValue expandMul(SDNode *N, SelectionDAG &DAG,
     if (ScaleShift >= 1 && ScaleShift < 4) {
       unsigned ShiftAmt = Log2_64(((MulAmt - 1) & (MulAmt - 2)));
       SDLoc DL(N);
-      SDValue Shift1 = DAG.getNode(ISD::SHL, DL, VT, N->getOperand(0),
-                                   DAG.getConstant(ShiftAmt, DL, VT));
-      SDValue Shift2 = DAG.getNode(ISD::SHL, DL, VT, N->getOperand(0),
-                                   DAG.getConstant(ScaleShift, DL, VT));
-      return DAG.getNode(
-          ISD::ADD, DL, VT, Shift1,
-          DAG.getNode(ISD::ADD, DL, VT, Shift2, N->getOperand(0)));
+      SDValue X = DAG.getFreeze(N->getOperand(0));
+      SDValue Shift1 =
+          DAG.getNode(ISD::SHL, DL, VT, X, DAG.getConstant(ShiftAmt, DL, VT));
+      SDValue Shift2 =
+          DAG.getNode(ISD::SHL, DL, VT, X, DAG.getConstant(ScaleShift, DL, VT));
+      return DAG.getNode(ISD::ADD, DL, VT, Shift1,
+                         DAG.getNode(ISD::ADD, DL, VT, Shift2, X));
     }
   }
 

@preames
Copy link
Collaborator Author

preames commented Apr 22, 2024

ping?

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@preames preames merged commit 9a35951 into llvm:main Apr 22, 2024
6 checks passed
@preames preames deleted the pr-riscv-mul-expand-freeze branch April 22, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants