-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[NVPTX] Use correct mul.wide operand type when matching on shl in combineMulWide
#168986
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR fixes a type mismatch in the NVPTX backend's combineMulWide function. The mul.wide instruction requires operands that are half the size of the output type, but the code was incorrectly using the output type (ToVT) instead of the operand type (FromVT) when creating constant operands for shift-to-multiply transformations.
Key Changes:
- Corrected the bit width used when creating
APIntconstants fromToVT.getSizeInBits()toFromVT.getSizeInBits() - Updated the type parameter in
getConstant()fromToVTtoFromVT
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@llvm/pr-subscribers-backend-nvptx Author: Justin Fargnoli (justinfargnoli) ChangesThe operand types of a Full diff: https://github.com/llvm/llvm-project/pull/168986.diff 1 Files Affected:
diff --git a/llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp b/llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
index a77eb0240e677..b843b8344d15c 100644
--- a/llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
+++ b/llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
@@ -5695,8 +5695,8 @@ static SDValue combineMulWide(SDNode *N, TargetLowering::DAGCombinerInfo &DCI,
SDValue RHS = Op.getOperand(1);
if (Op.getOpcode() == ISD::SHL) {
const auto ShiftAmt = Op.getConstantOperandVal(1);
- const auto MulVal = APInt(ToVT.getSizeInBits(), 1) << ShiftAmt;
- RHS = DCI.DAG.getConstant(MulVal, DL, ToVT);
+ const auto MulVal = APInt(FromVT.getSizeInBits(), 1) << ShiftAmt;
+ RHS = DCI.DAG.getConstant(MulVal, DL, FromVT);
}
return DCI.DAG.getNode(Opcode, DL, ToVT, Op.getOperand(0), RHS);
}
|
|
I don't have a test case that exposes this bug. I discovered this while prototyping another change. Since I'm not sure when that prototype will be ready for review, or whether it makes sense to land at all, I wanted to put up a fix and expose that this bug exists. |
mul.wide operand type when matching on shl in combineMulWidemul.wide operand type when matching on shl in combineMulWide
🐧 Linux x64 Test Results
|
AlexMaclean
left a comment
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
| const auto MulVal = APInt(FromVT.getSizeInBits(), 1) << ShiftAmt; | ||
| RHS = DCI.DAG.getConstant(MulVal, DL, FromVT); |
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.
It would be great to add a test that would catch such a mismatch.
The operand types of a
mul.wideare half the size of the output type.combineMulWideincorrectly uses the output type of themul.widefor the operand type.