-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
[SelectionDAG] Move SelectionDAG::getAllOnesConstant out of line. NFC #102995
Conversation
This function has to get the scalar size and create an APInt. I don't think it belong inline.
@llvm/pr-subscribers-llvm-selectiondag Author: Craig Topper (topperc) ChangesThis function has to get the scalar size and create an APInt. I don't think it belongs inline. Full diff: https://github.com/llvm/llvm-project/pull/102995.diff 2 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/SelectionDAG.h b/llvm/include/llvm/CodeGen/SelectionDAG.h
index 1d0124ec755352..9ce3c48a7f76cb 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAG.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAG.h
@@ -675,10 +675,7 @@ class SelectionDAG {
bool isTarget = false, bool isOpaque = false);
SDValue getAllOnesConstant(const SDLoc &DL, EVT VT, bool IsTarget = false,
- bool IsOpaque = false) {
- return getConstant(APInt::getAllOnes(VT.getScalarSizeInBits()), DL, VT,
- IsTarget, IsOpaque);
- }
+ bool IsOpaque = false);
SDValue getConstant(const ConstantInt &Val, const SDLoc &DL, EVT VT,
bool isTarget = false, bool isOpaque = false);
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index c3a7df5361cd45..fcf28f00a1008e 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -1748,6 +1748,12 @@ SDValue SelectionDAG::getConstant(const ConstantInt &Val, const SDLoc &DL,
return Result;
}
+SDValue SelectionDAG::getAllOnesConstant(const SDLoc &DL, EVT VT,
+ bool IsTarget, bool IsOpaque) {
+ return getConstant(APInt::getAllOnes(VT.getScalarSizeInBits()), DL, VT,
+ IsTarget, IsOpaque);
+}
+
SDValue SelectionDAG::getIntPtrConstant(uint64_t Val, const SDLoc &DL,
bool isTarget) {
return getConstant(Val, DL, TLI->getPointerTy(getDataLayout()), isTarget);
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
@@ -675,10 +675,7 @@ class SelectionDAG { | |||
bool isTarget = false, bool isOpaque = false); | |||
|
|||
SDValue getAllOnesConstant(const SDLoc &DL, EVT VT, bool IsTarget = false, | |||
bool IsOpaque = false) { | |||
return getConstant(APInt::getAllOnes(VT.getScalarSizeInBits()), DL, VT, |
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.
Can't this just use -1 and let the implementation extend to APInt if it's larger?
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.
Doing that is exactly how most i128 miscompiles get introduced ;) Note that getConstant() performs a zero extension, not a sign extension.
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.
But that sounds like a bug, generally constants are sign extended in other contexts in llvm (e.g. in MachineOperand)
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.
No, constants in LLVM are zero extended by default. They only get sign extended to 64 bit as a result of normal integer promotion rules. Some APIs have a flag to switch to sign extension instead, and everyone always forgets to set it when necessary, because things look like they work correctly for small integer types. (See also #80309.)
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
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
…llvm#102995) This function has to get the scalar size and create an APInt. I don't think it belongs inline.
…fbbce6c21 Local branch amd-gfx b0ffbbc Merged main:887f7002b65f7376c7a5004535bd08c95bdaa8f8 into amd-gfx:3ae0653ec5af Remote branch main f58f92c [SelectionDAG] Move SelectionDAG::getAllOnesConstant out of line. NFC (llvm#102995)
This function has to get the scalar size and create an APInt. I don't think it belongs inline.