ValueTracking: Use fcAllFlags for unknown value

[arsenm]

In the failure case we return null, which callers are checking. We were also returning an fcNone which was unused. It's more consistent to return fcAllFlags as any possible value, such that the value is always directly usable without checking the returned value.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Changes

In the failure case we return null, which callers are checking. We were also returning an fcNone which was unused. It's more consistent to return fcAllFlags as any possible value, such that the value is always directly usable without checking the returned value. -- Full diff: https://github.com//pull/66393.diff

2 Files Affected:

• (modified) llvm/lib/Analysis/ValueTracking.cpp (+8-8)
• (modified) llvm/unittests/Analysis/ValueTrackingTest.cpp (+4-4)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index c4153b824c37e0a..b0ec4e55034aa68 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -4008,7 +4008,7 @@ std::pair<Value *, FPClassTest> llvm::fcmpToClassTest(FCmpInst::Predicate Pred,
                                                       bool LookThroughSrc) {
   const APFloat *ConstRHS;
   if (!match(RHS, m_APFloatAllowUndef(ConstRHS)))
-    return {nullptr, fcNone};
+    return {nullptr, fcAllFlags};
 
   return fcmpToClassTest(Pred, F, LHS, ConstRHS, LookThroughSrc);
 }
@@ -4030,7 +4030,7 @@ llvm::fcmpToClassTest(FCmpInst::Predicate Pred, const Function &F, Value *LHS,
     // TODO: Handle DAZ by expanding masks to cover subnormal cases.
     if (Pred != FCmpInst::FCMP_ORD && Pred != FCmpInst::FCMP_UNO &&
         !inputDenormalIsIEEE(F, LHS->getType()))
-      return {nullptr, fcNone};
+      return {nullptr, fcAllFlags};
 
     switch (Pred) {
     case FCmpInst::FCMP_OEQ: // Match x == 0.0
@@ -4067,7 +4067,7 @@ llvm::fcmpToClassTest(FCmpInst::Predicate Pred, const Function &F, Value *LHS,
       break;
     }
 
-    return {nullptr, fcNone};
+    return {nullptr, fcAllFlags};
   }
 
   Value *Src = LHS;
@@ -4151,7 +4151,7 @@ llvm::fcmpToClassTest(FCmpInst::Predicate Pred, const Function &F, Value *LHS,
     case FCmpInst::FCMP_OGE:
     case FCmpInst::FCMP_ULT: {
       if (ConstRHS->isNegative()) // TODO
-        return {nullptr, fcNone};
+        return {nullptr, fcAllFlags};
 
       // fcmp oge fabs(x), +inf -> fcInf
       // fcmp oge x, +inf -> fcPosInf
@@ -4165,14 +4165,14 @@ llvm::fcmpToClassTest(FCmpInst::Predicate Pred, const Function &F, Value *LHS,
     case FCmpInst::FCMP_OGT:
     case FCmpInst::FCMP_ULE: {
       if (ConstRHS->isNegative())
-        return {nullptr, fcNone};
+        return {nullptr, fcAllFlags};
 
       // No value is ordered and greater than infinity.
       Mask = fcNone;
       break;
     }
     default:
-      return {nullptr, fcNone};
+      return {nullptr, fcAllFlags};
     }
   } else if (ConstRHS->isSmallestNormalized() && !ConstRHS->isNegative()) {
     // Match pattern that's used in __builtin_isnormal.
@@ -4201,14 +4201,14 @@ llvm::fcmpToClassTest(FCmpInst::Predicate Pred, const Function &F, Value *LHS,
       break;
     }
     default:
-      return {nullptr, fcNone};
+      return {nullptr, fcAllFlags};
     }
   } else if (ConstRHS->isNaN()) {
     // fcmp o__ x, nan -> false
     // fcmp u__ x, nan -> true
     Mask = fcNone;
   } else
-    return {nullptr, fcNone};
+    return {nullptr, fcAllFlags};
 
   // Invert the comparison for the unordered cases.
   if (FCmpInst::isUnordered(Pred))
diff --git a/llvm/unittests/Analysis/ValueTrackingTest.cpp b/llvm/unittests/Analysis/ValueTrackingTest.cpp
index 4eed22d224588f8..5bd1bb37e678e96 100644
--- a/llvm/unittests/Analysis/ValueTrackingTest.cpp
+++ b/llvm/unittests/Analysis/ValueTrackingTest.cpp
@@ -1848,13 +1848,13 @@ TEST_F(ComputeKnownFPClassTest, FCmpToClassTest_NInf) {
       fcmpToClassTest(CmpInst::FCMP_OGT, *A3->getFunction(), A3->getOperand(0),
                       A3->getOperand(1));
   EXPECT_EQ(nullptr, OgtVal);
-  EXPECT_EQ(fcNone, OgtClass);
+  EXPECT_EQ(fcAllFlags, OgtClass);
 
   auto [UleVal, UleClass] =
       fcmpToClassTest(CmpInst::FCMP_ULE, *A4->getFunction(), A4->getOperand(0),
                       A4->getOperand(1));
   EXPECT_EQ(nullptr, UleVal);
-  EXPECT_EQ(fcNone, UleClass);
+  EXPECT_EQ(fcAllFlags, UleClass);
 }
 
 TEST_F(ComputeKnownFPClassTest, FCmpToClassTest_PInf) {
@@ -1881,13 +1881,13 @@ TEST_F(ComputeKnownFPClassTest, FCmpToClassTest_PInf) {
       fcmpToClassTest(CmpInst::FCMP_OLE, *A3->getFunction(), A3->getOperand(0),
                       A3->getOperand(1));
   EXPECT_EQ(nullptr, OleVal);
-  EXPECT_EQ(fcNone, OleClass);
+  EXPECT_EQ(fcAllFlags, OleClass);
 
   auto [UgtVal, UgtClass] =
       fcmpToClassTest(CmpInst::FCMP_UGT, *A4->getFunction(), A4->getOperand(0),
                       A4->getOperand(1));
   EXPECT_EQ(nullptr, UgtVal);
-  EXPECT_EQ(fcNone, UgtClass);
+  EXPECT_EQ(fcAllFlags, UgtClass);
 }
 
 TEST_F(ComputeKnownFPClassTest, SqrtNszSignBit) {

[jayfoad]

Do you have an example of how this would simplify some caller?

[arsenm]

Do you have an example of how this would simplify some caller?

This just sort of ends up happening in my attempt to merge fcmpToClassTest and fcmpImpliesClass after #65983

[jayfoad]

Well I still don't get what kind of caller could use the returned mask without also checking the returned Value. An example might help.

[arsenm]

Well I still don't get what kind of caller could use the returned mask without also checking the returned Value. An example might help.

I'm not actually doing this for the benefit of the caller, I'm trying to avoid confusion in the implementation itself

[jayfoad]

Well I still don't get what kind of caller could use the returned mask without also checking the returned Value. An example might help.

I'm not actually doing this for the benefit of the caller, I'm trying to avoid confusion in the implementation itself

Then I don't get what you meant by "such that the value is always directly usable without checking the returned value".

[arsenm]

Well I still don't get what kind of caller could use the returned mask without also checking the returned Value. An example might help.

I'm not actually doing this for the benefit of the caller, I'm trying to avoid confusion in the implementation itself

Then I don't get what you meant by "such that the value is always directly usable without checking the returned value".

I mean fcAllFlags is the correct conservative value to use for the unknown case. fcNone is extremely aggressive

[jayfoad]

Well I don't object to the patch but it sounds like it has no effect on anything.

[arsenm]

Well I don't object to the patch but it sounds like it has no effect on anything.

Yes, it's NFC but splits out a behavior change from a future patch

[nikic]

I don't really get when this could matter (if you don't have an operand, then there is nothing the mask could apply to), but I also agree that this is slightly nicer by dint of being the conservative return value.