Skip to content

[InstSimplify] fold uno/ord comparison if fpclass is always NaN #97763

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 3 commits into from
Jul 9, 2024

Conversation

AlexMaclean
Copy link
Member

@AlexMaclean AlexMaclean commented Jul 4, 2024

In InstSimplify we already fold fcmp ord/uno to a constant when both operands are known to be non-NaN. This change slightly generalizes this to also handle the case where either of the operands is known to always be NaN.

Proof: https://alive2.llvm.org/ce/z/AhCmJN

@AlexMaclean AlexMaclean requested a review from dtcxzyw July 4, 2024 20:36
@AlexMaclean AlexMaclean self-assigned this Jul 4, 2024
@AlexMaclean AlexMaclean requested a review from nikic as a code owner July 4, 2024 20:36
@llvmbot llvmbot added llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Jul 4, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 4, 2024

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Alex MacLean (AlexMaclean)

Changes

We already canonicalize operands that are known to be never NaN to 0.0 for fcmp ord/uno. This change slightly generalizes this to also handle operands which are known to always be NaN by canonicalizing them to a NaN constant.


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

3 Files Affected:

  • (modified) llvm/include/llvm/Analysis/ValueTracking.h (+5)
  • (modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+14-7)
  • (modified) llvm/test/Transforms/InstCombine/fcmp-special.ll (+16)
diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index a67ad501982d2..51db54aa9b61d 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -276,6 +276,8 @@ struct KnownFPClass {
     return (KnownFPClasses & Mask) == fcNone;
   }
 
+  bool isKnownAlways(FPClassTest Mask) const { return isKnownNever(~Mask); }
+
   bool isUnknown() const {
     return KnownFPClasses == fcAllFlags && !SignBit;
   }
@@ -285,6 +287,9 @@ struct KnownFPClass {
     return isKnownNever(fcNan);
   }
 
+  /// Return true if it's known this must always be a nan.
+  bool isKnownAlwaysNaN() const { return isKnownAlways(fcNan); }
+
   /// Return true if it's known this can never be an infinity.
   bool isKnownNeverInfinity() const {
     return isKnownNever(fcInf);
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index abadf54a96767..040dcab931068 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -8103,13 +8103,20 @@ Instruction *InstCombinerImpl::visitFCmpInst(FCmpInst &I) {
   // If we're just checking for a NaN (ORD/UNO) and have a non-NaN operand,
   // then canonicalize the operand to 0.0.
   if (Pred == CmpInst::FCMP_ORD || Pred == CmpInst::FCMP_UNO) {
-    if (!match(Op0, m_PosZeroFP()) &&
-        isKnownNeverNaN(Op0, 0, getSimplifyQuery().getWithInstruction(&I)))
-      return replaceOperand(I, 0, ConstantFP::getZero(OpType));
-
-    if (!match(Op1, m_PosZeroFP()) &&
-        isKnownNeverNaN(Op1, 0, getSimplifyQuery().getWithInstruction(&I)))
-      return replaceOperand(I, 1, ConstantFP::getZero(OpType));
+    if (!match(Op0, m_PosZeroFP())) {
+      KnownFPClass Known0 = computeKnownFPClass(Op0, fcAllFlags, &I);
+      if (Known0.isKnownNeverNaN())
+        return replaceOperand(I, 0, ConstantFP::getZero(OpType));
+      if (Known0.isKnownAlwaysNaN())
+        return replaceOperand(I, 0, ConstantFP::getNaN(OpType));
+    }
+    if (!match(Op1, m_PosZeroFP())) {
+      KnownFPClass Known1 = computeKnownFPClass(Op1, fcAllFlags, &I);
+      if (Known1.isKnownNeverNaN())
+        return replaceOperand(I, 1, ConstantFP::getZero(OpType));
+      if (Known1.isKnownAlwaysNaN())
+        return replaceOperand(I, 1, ConstantFP::getNaN(OpType));
+    }
   }
 
   // fcmp pred (fneg X), (fneg Y) -> fcmp swap(pred) X, Y
diff --git a/llvm/test/Transforms/InstCombine/fcmp-special.ll b/llvm/test/Transforms/InstCombine/fcmp-special.ll
index 64bc86f4266c7..6c196534931f5 100644
--- a/llvm/test/Transforms/InstCombine/fcmp-special.ll
+++ b/llvm/test/Transforms/InstCombine/fcmp-special.ll
@@ -186,6 +186,22 @@ define i1 @nnan_ops_to_fcmp_uno(float %x, float %y) {
   ret i1 %cmp
 }
 
+define i1 @nan_ops_to_fcmp_ord(float nofpclass(inf zero sub norm) %x, float nofpclass(inf zero sub norm) %y) {
+; CHECK-LABEL: @nan_ops_to_fcmp_ord(
+; CHECK-NEXT:    ret i1 false
+;
+  %cmp = fcmp ord float %x, %y
+  ret i1 %cmp
+}
+
+define i1 @nan_ops_to_fcmp_uno(float nofpclass(inf zero sub norm) %x, float nofpclass(inf zero sub norm) %y) {
+; CHECK-LABEL: @nan_ops_to_fcmp_uno(
+; CHECK-NEXT:    ret i1 true
+;
+  %cmp = fcmp uno float %x, %y
+  ret i1 %cmp
+}
+
 ; TODO: For any predicate/type/FMF, comparison to -0.0 is the same as comparison to +0.0.
 
 define i1 @negative_zero_oeq(float %x) {

@dtcxzyw dtcxzyw requested a review from arsenm July 5, 2024 03:40
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.

Please attach alive2 proofs and pre-commit tests.

@AlexMaclean
Copy link
Member Author

Please attach alive2 proofs and pre-commit tests.

Okay, I've attached a proof, could you explain further about what you mean by pre-commit tests?

@nikic
Copy link
Contributor

nikic commented Jul 5, 2024

Please attach alive2 proofs and pre-commit tests.

Okay, I've attached a proof, could you explain further about what you mean by pre-commit tests?

See https://llvm.org/docs/InstCombineContributorGuide.html#precommit-tests.

@AlexMaclean AlexMaclean force-pushed the upstream/fcmp-known-nan branch from e7a5762 to add8cee Compare July 5, 2024 19:55
@AlexMaclean
Copy link
Member Author

Please attach alive2 proofs and pre-commit tests.

Okay, I've attached a proof, could you explain further about what you mean by pre-commit tests?

See https://llvm.org/docs/InstCombineContributorGuide.html#precommit-tests.

Thanks, I've split the tests out into a separate commit as described, please let me know if I've made any mistake.

@dtcxzyw
Copy link
Member

dtcxzyw commented Jul 6, 2024

Please update the title and description, since this PR folds the pattern into a constant instead of canonicalizing its operands.

@AlexMaclean AlexMaclean force-pushed the upstream/fcmp-known-nan branch from add8cee to 733fa90 Compare July 6, 2024 16:49
@AlexMaclean AlexMaclean changed the title [InstCombine] canonicalize operands of fcmp ord/uno if they are known NaN [InstSimplify] fold uno/ord comparison if fpclass is always NaN Jul 6, 2024
@AlexMaclean AlexMaclean force-pushed the upstream/fcmp-known-nan branch from 733fa90 to b54f92f Compare July 6, 2024 18:02
dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Jul 7, 2024
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.

LGTM. Please wait for additional approval from other reviewers :)

@AlexMaclean
Copy link
Member Author

@nikic, @arsenm could I get one of your stamps as well before I land?

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

@AlexMaclean AlexMaclean merged commit 8e9d50c into llvm:main Jul 9, 2024
7 checks passed
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
…#97763)

In InstSimplify we already fold `fcmp ord/uno` to a constant when both
operands are known to be non-NaN. This change slightly generalizes this
to also handle the case where either of the operands is known to always
be NaN.

Proof: https://alive2.llvm.org/ce/z/AhCmJN
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants