Skip to content

Conversation

RalfJung
Copy link
Contributor

@RalfJung RalfJung commented Oct 8, 2024

I noticed this while following #111269. It makes little sense that FCOPYSIGN would look at the sign of x, right? Surely this must be y. Also fix the inconsistency where it's sometimes x and sometimes X.

@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Oct 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 8, 2024

@llvm/pr-subscribers-llvm-selectiondag

Author: Ralf Jung (RalfJung)

Changes

I noticed this while following #111269. It makes little sense that FCOPYSIGN would look at the sign of x, right? Surely this must be y. Also fix the inconsistency where it's sometimes x and sometimes X.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp (+1-1)
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
index 42d031310d5e02..5d324f50cce457 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
@@ -1657,7 +1657,7 @@ SDValue SelectionDAGLegalize::ExpandFCOPYSIGN(SDNode *Node) const {
   SDValue SignBit = DAG.getNode(ISD::AND, DL, IntVT, SignAsInt.IntValue,
                                 SignMask);
 
-  // If FABS is legal transform FCOPYSIGN(x, y) => sign(x) ? -FABS(x) : FABS(X)
+  // If FABS is legal transform FCOPYSIGN(x, y) => SignBit(y) ? -FABS(x) : FABS(x)
   EVT FloatVT = Mag.getValueType();
   if (TLI.isOperationLegalOrCustom(ISD::FABS, FloatVT) &&
       TLI.isOperationLegalOrCustom(ISD::FNEG, FloatVT)) {

Copy link

github-actions bot commented Oct 8, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

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

Nice catch! Please fix format before pushing.

@arsenm arsenm added the floating-point Floating-point math label Oct 8, 2024
@arsenm arsenm changed the title fix comment typo in ExpandFCOPYSIGN Fix comment typo in ExpandFCOPYSIGN Oct 8, 2024
@RalfJung
Copy link
Contributor Author

RalfJung commented Oct 8, 2024 via email

@arsenm arsenm merged commit 29ec071 into llvm:main Oct 8, 2024
10 checks passed
@RalfJung RalfJung deleted the ExpandFCOPYSIGN branch October 11, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
floating-point Floating-point math llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants