Skip to content

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Jul 24, 2025

Spotted while reviewing #150211. If we're multiplying by -3 in i32 MulAmt contains 4,294,967,293 since we zero extend to uint64_t. Adding 3 to this gives 0x100000000 which is a power of 2 and the log2 of that is 32, but we can't shift left by 32 in an i32.

Detect this case and skip the transform. We could use 0, but we don't handle the case for i64 so this seemed more consistent.

Normally we don't hit this case because decomposeMulByConstant handles it, but that's disabled by Xqciac. And after #150211 the path in expandMul will also be unreachable. So I didn't add a test to avoid messing with that review.

Spotted while reviewing llvm#150211. If we're multiplying by -3 in i32
MulAmt contains 4,294,967,293 since we zero extend to uint64_t.
Adding 3 to this gives 0x100000000 which is a power of 2 and the log2
of that is 32, but we can't shift left by 32 in an i32.

We should use 0 instead of the shl in this case.

Normally we don't hit this case because decomeMulByConstant handles
it, but that's disabled by Qciac. And after llvm#150211 the path in
expandMul will also be unreachable. So I didn't add a test to avoid
messing with that review.
@llvmbot
Copy link
Member

llvmbot commented Jul 24, 2025

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

Author: Craig Topper (topperc)

Changes

Spotted while reviewing #150211. If we're multiplying by -3 in i32 MulAmt contains 4,294,967,293 since we zero extend to uint64_t. Adding 3 to this gives 0x100000000 which is a power of 2 and the log2 of that is 32, but we can't shift left by 32 in an i32.

We should use 0 instead of the shl in this case.

Normally we don't hit this case because decomeMulByConstant handles it, but that's disabled by Qciac. And after #150211 the path in expandMul will also be unreachable. So I didn't add a test to avoid messing with that review.


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

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+7-2)
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 3918dd21bc09d..25cb230785f72 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -16184,10 +16184,15 @@ static SDValue expandMul(SDNode *N, SelectionDAG &DAG,
     // 2^N - 3/5/9 --> (sub (shl X, C1), (shXadd X, x))
     for (uint64_t Offset : {3, 5, 9}) {
       if (isPowerOf2_64(MulAmt + Offset)) {
+        unsigned ShAmt = Log2_64(MulAmt + Offset);
         SDLoc DL(N);
-        SDValue Shift1 =
+        SDValue Shift1;
+        if (ShAmt >= VT.getSizeInBits())
+          Shift1 = DAG.getConstant(0, DL, VT);
+        else
+          Shift1 =
             DAG.getNode(ISD::SHL, DL, VT, X,
-                        DAG.getConstant(Log2_64(MulAmt + Offset), DL, VT));
+                        DAG.getConstant(ShAmt, DL, VT));
         SDValue Mul359 =
             DAG.getNode(RISCVISD::SHL_ADD, DL, VT, X,
                         DAG.getConstant(Log2_64(Offset - 1), DL, VT), X);

Copy link
Contributor

@svs-quic svs-quic left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this.

@topperc topperc merged commit ae36702 into llvm:main Jul 25, 2025
9 checks passed
@topperc topperc deleted the pr/expandmul branch July 25, 2025 00:27
mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Jul 28, 2025
Spotted while reviewing llvm#150211. If we're multiplying by -3 in i32
MulAmt contains 4,294,967,293 since we zero extend to uint64_t. Adding 3
to this gives 0x100000000 which is a power of 2 and the log2 of that is
32, but we can't shift left by 32 in an i32.

Detect this case and skip the transform. We could use 0, but we don't
handle the case for i64 so this seemed more consistent.

Normally we don't hit this case because decomposeMulByConstant handles
it, but that's disabled by Xqciac. And after llvm#150211 the code in
expandMul is now unreachable for this case.
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.

3 participants