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

[SelectionDAG] Move SelectionDAG::getAllOnesConstant out of line. NFC #102995

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Aug 13, 2024

This function has to get the scalar size and create an APInt. I don't think it belongs inline.

This function has to get the scalar size and create an APInt. I
don't think it belong inline.
@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Aug 13, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 13, 2024

@llvm/pr-subscribers-llvm-selectiondag

Author: Craig Topper (topperc)

Changes

This 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:

  • (modified) llvm/include/llvm/CodeGen/SelectionDAG.h (+1-4)
  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+6)
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);

Copy link

github-actions bot commented Aug 13, 2024

✅ 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,
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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)

Copy link
Contributor

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.)

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

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

LGTM

@topperc topperc merged commit f58f92c into llvm:main Aug 13, 2024
8 checks passed
@topperc topperc deleted the pr/getallonesconstan branch August 13, 2024 17:15
bwendling pushed a commit to bwendling/llvm-project that referenced this pull request Aug 15, 2024
…llvm#102995)

This function has to get the scalar size and create an APInt. I don't
think it belongs inline.
qiaojbao pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Aug 29, 2024
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants