Skip to content

DAG: Lower fcNormal is.fpclass to compare with inf #100389

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

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

arsenm
Copy link
Contributor

@arsenm arsenm commented Jul 24, 2024

Looks worse for x86 without the fabs check. Not sure if
this is useful for any targets.

Copy link
Contributor Author

arsenm commented Jul 24, 2024

@llvmbot
Copy link
Member

llvmbot commented Jul 24, 2024

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: Matt Arsenault (arsenm)

Changes

Looks worse for x86 without the fabs check. Not sure if
this is useful for any targets.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp (+25)
diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index 0036c182ab9db..4227f7bec9ec8 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -8652,6 +8652,31 @@ SDValue TargetLowering::expandIS_FPCLASS(EVT ResultVT, SDValue Op,
       return DAG.getSetCC(DL, ResultVT, Op, Inf,
                           IsOrdered ? OrderedCmpOpcode : UnorderedCmpOpcode);
     }
+
+    if (FPTestMask == fcNormal) {
+      // TODO: Handle unordered
+      ISD::CondCode IsFiniteOp = IsInvertedFP ? ISD::SETUGE : ISD::SETOLT;
+      ISD::CondCode IsNormalOp = IsInvertedFP ? ISD::SETOLT : ISD::SETUGE;
+
+      if (isCondCodeLegalOrCustom(IsFiniteOp,
+                                  OperandVT.getScalarType().getSimpleVT()) &&
+          isCondCodeLegalOrCustom(IsNormalOp,
+                                  OperandVT.getScalarType().getSimpleVT()) &&
+          isFAbsFree(OperandVT)) {
+        // isnormal(x) --> fabs(x) < infinity && !(fabs(x) < smallest_normal)
+        SDValue Inf =
+            DAG.getConstantFP(APFloat::getInf(Semantics), DL, OperandVT);
+        SDValue SmallestNormal = DAG.getConstantFP(
+            APFloat::getSmallestNormalized(Semantics), DL, OperandVT);
+
+        SDValue Abs = DAG.getNode(ISD::FABS, DL, OperandVT, Op);
+        SDValue IsFinite = DAG.getSetCC(DL, ResultVT, Abs, Inf, IsFiniteOp);
+        SDValue IsNormal =
+            DAG.getSetCC(DL, ResultVT, Abs, SmallestNormal, IsNormalOp);
+        unsigned LogicOp = IsInvertedFP ? ISD::OR : ISD::AND;
+        return DAG.getNode(LogicOp, DL, ResultVT, IsFinite, IsNormal);
+      }
+    }
   }
 
   // Some checks may be represented as inversion of simpler check, for example

@jayfoad
Copy link
Contributor

jayfoad commented Jul 24, 2024

Looks worse for x86 without the fabs check. Not sure if this is useful for any targets.

Seems unlikely that this would ever be profitable in the ordered case, since you can implement that with pretty simple integer checks on the exponent field. (Check that it isn't 0 and isn't maximal.)

@arsenm arsenm force-pushed the users/arsenm/dag-lower-single-inf-is-fpclass-to-fcmp branch from 1b48c68 to fc46244 Compare July 26, 2024 11:55
@arsenm arsenm force-pushed the users/arsenm/dag-lower-isfpclass-normal-to-fcmp-inf branch from 257cb80 to fcfbc51 Compare July 26, 2024 11:55
@arsenm arsenm force-pushed the users/arsenm/dag-lower-single-inf-is-fpclass-to-fcmp branch from fc46244 to 6226f31 Compare July 26, 2024 18:48
@arsenm arsenm force-pushed the users/arsenm/dag-lower-isfpclass-normal-to-fcmp-inf branch from fcfbc51 to f515257 Compare July 26, 2024 18:49
@arsenm arsenm force-pushed the users/arsenm/dag-lower-single-inf-is-fpclass-to-fcmp branch from 6226f31 to eaf47a2 Compare August 22, 2024 06:17
@arsenm arsenm force-pushed the users/arsenm/dag-lower-isfpclass-normal-to-fcmp-inf branch from f515257 to 1e2a2b6 Compare August 22, 2024 06:18
@arsenm arsenm force-pushed the users/arsenm/dag-lower-single-inf-is-fpclass-to-fcmp branch from eaf47a2 to 7d48a38 Compare August 29, 2024 04:19
@arsenm arsenm force-pushed the users/arsenm/dag-lower-isfpclass-normal-to-fcmp-inf branch from 1e2a2b6 to f5da092 Compare August 29, 2024 04:19
@arsenm arsenm force-pushed the users/arsenm/dag-lower-single-inf-is-fpclass-to-fcmp branch from 7d48a38 to f4df5b3 Compare August 29, 2024 10:08
@arsenm arsenm force-pushed the users/arsenm/dag-lower-isfpclass-normal-to-fcmp-inf branch from f5da092 to 9061a14 Compare August 29, 2024 10:09
@arsenm arsenm force-pushed the users/arsenm/dag-lower-single-inf-is-fpclass-to-fcmp branch from f4df5b3 to 226d977 Compare September 5, 2024 16:01
@arsenm arsenm force-pushed the users/arsenm/dag-lower-isfpclass-normal-to-fcmp-inf branch from 9061a14 to d51a155 Compare September 5, 2024 16:02
Base automatically changed from users/arsenm/dag-lower-single-inf-is-fpclass-to-fcmp to main September 6, 2024 05:15
Looks worse for x86 without the fabs check. Not sure if
this is useful for any targets.
@arsenm arsenm force-pushed the users/arsenm/dag-lower-isfpclass-normal-to-fcmp-inf branch from d51a155 to 9325186 Compare September 6, 2024 05:16
Copy link
Contributor Author

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

ping

@RKSimon
Copy link
Collaborator

RKSimon commented Sep 6, 2024

test coverage?

@arsenm
Copy link
Contributor Author

arsenm commented Sep 6, 2024

test coverage?

This won't trigger on any target I know of

@RKSimon
Copy link
Collaborator

RKSimon commented Sep 6, 2024

test coverage?

This won't trigger on any target I know of

So why bother?

@arsenm
Copy link
Contributor Author

arsenm commented Sep 11, 2024

So why bother?

Completeness of inverting the instcombine transform, and I thought it would help but depends

@arsenm
Copy link
Contributor Author

arsenm commented Oct 17, 2024

ping

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.

No objections

@arsenm arsenm merged commit 067e8b8 into main Oct 17, 2024
8 checks passed
@arsenm arsenm deleted the users/arsenm/dag-lower-isfpclass-normal-to-fcmp-inf branch October 17, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 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