[RISCV] Pass sign-extended value to isInt check in expandMul #150211
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the isInt check that was added in # we were passing the zero-extended uint64_t value instead of the sign-extended one.
|
@llvm/pr-subscribers-backend-risc-v Author: Sudharsan Veeravalli (svs-quic) ChangesIn the Full diff: https://github.com/llvm/llvm-project/pull/150211.diff 1 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 3918dd21bc09d..fa0196ff25339 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -16079,7 +16079,7 @@ static SDValue expandMul(SDNode *N, SelectionDAG &DAG,
uint64_t MulAmt = CNode->getZExtValue();
// Don't do this if the Xqciac extension is enabled and the MulAmt in simm12.
- if (Subtarget.hasVendorXqciac() && isInt<12>(MulAmt))
+ if (Subtarget.hasVendorXqciac() && isInt<12>(CNode->getSExtValue()))
return SDValue();
const bool HasShlAdd = Subtarget.hasStdExtZba() ||
|
|
Is it possible to test? |
Have added a test now. |
topperc
reviewed
Jul 24, 2025
topperc
added a commit
to topperc/llvm-project
that referenced
this pull request
Jul 24, 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. 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.
|
I think we should backport this to the release branch. |
lenary
reviewed
Jul 24, 2025
|
/cherry-pick d3937e2 |
|
/pull-request #150556 |
topperc
added a commit
that referenced
this pull request
Jul 25, 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 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the
isIntcheck that was added in #147661 we were passing the zero-extendeduint64_tvalue instead of the sign-extended one.