llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

[InstCombine] canonicalize operands of fcmp ord/uno if they are known NaN

Open AlexMaclean opened this issue 1 year ago • 1 comments

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.

AlexMaclean avatar Jul 04 '24 20:07 AlexMaclean

@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) {

llvmbot avatar Jul 04 '24 20:07 llvmbot

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?

AlexMaclean avatar Jul 05 '24 19:07 AlexMaclean

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.

nikic avatar Jul 05 '24 19:07 nikic

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.

AlexMaclean avatar Jul 05 '24 20:07 AlexMaclean

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

dtcxzyw avatar Jul 06 '24 03:07 dtcxzyw

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

AlexMaclean avatar Jul 09 '24 15:07 AlexMaclean