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

[LegalizeDAG] PromoteNode FABS: Override promotion with clear sign bit #106076

Closed
wants to merge 1 commit into from

Conversation

v01dXYZ
Copy link
Contributor

@v01dXYZ v01dXYZ commented Aug 26, 2024

Conditionally to the same sized integer type being legal.

For example, for f16, if i16 is legal, bitcast to i16, clear the sign bit and bitcast back to f16.

Fixes #104915

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

llvmbot commented Aug 26, 2024

@llvm/pr-subscribers-llvm-selectiondag

Author: None (v01dXYZ)

Changes

Conditionally to the same sized integer type being legal.

For example, for f16, if i16 is legal, bitcast to i16, clear the sign bit and bitcast back to f16.

Fixes #104915


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp (+21-1)
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
index 74e3a898569bea..fe8fb5d45676b5 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
@@ -5576,6 +5576,27 @@ void SelectionDAGLegalize::PromoteNode(SDNode *Node) {
     Results.push_back(Tmp2.getValue(1));
     break;
   }
+  case ISD::FABS: {
+    // if we can convert to a same sized int value, we bitcast to it,
+    // clear the sign bit and bitcast back to the original type.
+    EVT IntVT = OVT.changeTypeToInteger();
+    if (TLI.isTypeLegal(IntVT)) {
+      SDValue Cast = DAG.getNode(ISD::BITCAST, dl, IntVT, Node->getOperand(0));
+
+      APInt SignMask = APInt::getSignMask(IntVT.getScalarSizeInBits());
+
+      // Mask = ~(1 << (Size-1))
+      SDValue Mask = DAG.getConstant(SignMask, dl, IntVT);
+
+      SDValue And = DAG.getNode(ISD::AND, dl, IntVT, Cast, Mask);
+
+      SDValue Result = DAG.getNode(ISD::BITCAST, dl, OVT, And);
+      Results.push_back(Result);
+      break;
+    }
+
+    [[fallthrough]];
+  }
   case ISD::FFLOOR:
   case ISD::FCEIL:
   case ISD::FRINT:
@@ -5597,7 +5618,6 @@ void SelectionDAGLegalize::PromoteNode(SDNode *Node) {
   case ISD::FLOG:
   case ISD::FLOG2:
   case ISD::FLOG10:
-  case ISD::FABS:
   case ISD::FEXP:
   case ISD::FEXP2:
   case ISD::FEXP10:

Conditionally to the same sized integer type being legal.

For example, for f16, if i16 is legal, bitcast to i16, clear the sign
bit and bitcast back to f16.

Adapted from a suggested patch from @arsenm (Matthew.Arsenault@amd.com):

llvm#104915 (comment)
@v01dXYZ v01dXYZ force-pushed the 104915-legalize-dag-promote-fabs branch from 578feca to b42e049 Compare August 26, 2024 13:59
@dtcxzyw dtcxzyw requested review from arsenm, RKSimon and topperc August 26, 2024 14:42
@v01dXYZ
Copy link
Contributor Author

v01dXYZ commented Aug 26, 2024

I've got some problems with updating the NVPTX tests as the tests use asm-verbose=false but UpdateTestChecks/asm.py uses -- End Function.

@topperc
Copy link
Collaborator

topperc commented Aug 26, 2024

Why not use Expand instead of Promote for ISD::FABS in that case? It already has the code for bitcasting to int.

@v01dXYZ
Copy link
Contributor Author

v01dXYZ commented Aug 26, 2024

@topperc that's a valid point (even though I thought that expand means using smaller types to compute the result).

@topperc
Copy link
Collaborator

topperc commented Aug 26, 2024

@topperc that's a valid point (even though I thought that expand means using smaller types to compute the result).

That's what Expand means for LegalizeTypes. For LegalizeDAG, Expand just means to emulate it any way we can.

@v01dXYZ
Copy link
Contributor Author

v01dXYZ commented Aug 26, 2024

@topperc Thanks. I'll instead publish a PR for the targets that have i16 and set FABS/f16 action to Promote.

I'll put back this PR to Draft.

@v01dXYZ v01dXYZ marked this pull request as draft August 26, 2024 17:17
@v01dXYZ
Copy link
Contributor Author

v01dXYZ commented Aug 26, 2024

This PR has been replaced by #106153 which does the same without adding a special case for PromoteNode (and virtually no code).

@v01dXYZ v01dXYZ closed this Aug 26, 2024
@arsenm
Copy link
Contributor

arsenm commented Aug 28, 2024

This should be resurrected. The promotion to float through fpext is simply wrong. There should be no fall through for it, it should fail

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.

[X86] LLVM >= 18 folding bitcast -> and -> bitcast to @llvm.fabs.f16 generates call to __extendhfsf2
4 participants