ValueTracking: Ignore -0 for nsz sqrt with UseInstrInfo in computeKnownFPClass

[arsenm]

Summary

This avoids a regression when SignBitMustBeZero is moved to computeKnownFPClass.

[foad]

Looks OK but shouldn't the nsz flag be applied more generally to the result of every computeKnownFPClass call?

[arsenm]

In D151163#4363911, @foad wrote:

Looks OK but shouldn't the nsz flag be applied more generally to the result of every computeKnownFPClass call?

In D151163#4363911, @foad wrote:

Looks OK but shouldn't the nsz flag be applied more generally to the result of every computeKnownFPClass call?

No because of nsz’s bizarre definition. It doesn’t mean no -0 appears, it means you don’t care about the sign of zero.

[arsenm]

In D151163#4363921, @arsenm wrote:

In D151163#4363911, @foad wrote:

Looks OK but shouldn't the nsz flag be applied more generally to the result of every computeKnownFPClass call?

In D151163#4363911, @foad wrote:

Looks OK but shouldn't the nsz flag be applied more generally to the result of every computeKnownFPClass call?

No because of nsz’s bizarre definition. It doesn’t mean no -0 appears, it means you don’t care about the sign of zero.

This might be too strong as it is since it implies poison for -0, but I’m guessing that’s what the UseInstrInfo is for. Otherwise we would need another flag for stronger nsz interpretation

[arsenm]

ping

[arsenm]

ping

[jcranmer-intel]

In D151163#4363911, @foad wrote:

Looks OK but shouldn't the nsz flag be applied more generally to the result of every computeKnownFPClass call?

No because of nsz’s bizarre definition. It doesn’t mean no -0 appears, it means you don’t care about the sign of zero.

This might be too strong as it is since it implies poison for -0, but I’m guessing that’s what the UseInstrInfo is for. Otherwise we would need another flag for stronger nsz interpretation

I think the user intent of nsz is primarily to allow optimizations that are legal for every value except -0.0, and the use case here seems to me to be clearly that (sqrt(x) has sign=0 for every x but -0.0). It's not immediately clear to me that you can generalize handling of nsz for all cases in computeKnownFPClass.

[arsenm]

06f942016b6b1e376334937a3b132232e311104f

llvm/include/llvm/Analysis/InstructionSimplify.h

template <class InstT> bool hasNoSignedWrap(const InstT *Op) const {
     return false;
   }
 
   bool isExact(const BinaryOperator *Op) const {
     if (UseInstrInfo && isa<PossiblyExactOperator>(Op))
       return cast<PossiblyExactOperator>(Op)->isExact();
     return false;
   }
+
+  template <class InstT> bool hasNoSignedZeros(const InstT *Op) const {
+    if (UseInstrInfo)
+      return Op->hasNoSignedZeros();
+    return false;
+  }
 };
 
 struct SimplifyQuery {
   const DataLayout &DL;
   const TargetLibraryInfo *TLI = nullptr;
   const DominatorTree *DT = nullptr;
   AssumptionCache *AC = nullptr;
   const Instruction *CxtI = nullptr;

llvm/lib/Analysis/ValueTracking.cpp

if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(Op)) {
           Known.knownNot(fcNan);
 
         // The only negative value that can be returned is -0 for -0 inputs.
         Known.knownNot(fcNegInf | fcNegSubnormal | fcNegNormal);
 
         // If the input denormal mode could be PreserveSign, a negative
         // subnormal input could produce a negative zero output.
         const Function *F = II->getFunction();
-        if (F && KnownSrc.isKnownNeverLogicalNegZero(*F, II->getType())) {
+        if (Q.IIQ.hasNoSignedZeros(II) ||
+            (F && KnownSrc.isKnownNeverLogicalNegZero(*F, II->getType()))) {
           Known.knownNot(fcNegZero);
           if (KnownSrc.isKnownNeverNaN())
             Known.SignBit = false;
         }
 
         break;
       }
       case Intrinsic::sin:

llvm/unittests/Analysis/ValueTrackingTest.cpp

TEST_F(ComputeKnownFPClassTest, CannotBeOrderedLessThanZero) {
   EXPECT_TRUE(
       computeKnownFPClass(ConstantFP::getSNaN(FPTy, /*Negative=*/false), DL)
           .cannotBeOrderedLessThanZero());
   EXPECT_TRUE(
       computeKnownFPClass(ConstantFP::getSNaN(FPTy, /*Negative=*/true), DL)
           .cannotBeOrderedLessThanZero());
 }
 
+TEST_F(ComputeKnownFPClassTest, SqrtNszSignBit) {
+  parseAssembly(
+      "declare float @llvm.sqrt.f32(float)\n"
+      "define float @test(float %arg, float nofpclass(nan) %arg.nnan) {\n"
+      "  %A = call float @llvm.sqrt.f32(float %arg)\n"
+      "  %A2 = call nsz float @llvm.sqrt.f32(float %arg)\n"
+      "  %A3 = call float @llvm.sqrt.f32(float %arg.nnan)\n"
+      "  %A4 = call nsz float @llvm.sqrt.f32(float %arg.nnan)\n"
+      "  ret float %A\n"
+      "}\n");
+
+  const FPClassTest SqrtMask = fcPositive | fcNegZero | fcNan;
+  const FPClassTest NszSqrtMask = fcPositive | fcNan;
+
+  {
+    KnownFPClass UseInstrInfo = computeKnownFPClass(
+        A, M->getDataLayout(), fcAllFlags, 0, nullptr, nullptr, nullptr,
+        nullptr, nullptr, /*UseInstrInfo=*/true);
+    EXPECT_EQ(SqrtMask, UseInstrInfo.KnownFPClasses);
+    EXPECT_EQ(std::nullopt, UseInstrInfo.SignBit);
+
+    KnownFPClass NoUseInstrInfo = computeKnownFPClass(
+        A, M->getDataLayout(), fcAllFlags, 0, nullptr, nullptr, nullptr,
+        nullptr, nullptr, /*UseInstrInfo=*/false);
+    EXPECT_EQ(SqrtMask, NoUseInstrInfo.KnownFPClasses);
+    EXPECT_EQ(std::nullopt, NoUseInstrInfo.SignBit);
+  }
+
+  {
+    KnownFPClass UseInstrInfoNSZ = computeKnownFPClass(
+        A2, M->getDataLayout(), fcAllFlags, 0, nullptr, nullptr, nullptr,
+        nullptr, nullptr, /*UseInstrInfo=*/true);
+    EXPECT_EQ(NszSqrtMask, UseInstrInfoNSZ.KnownFPClasses);
+    EXPECT_EQ(std::nullopt, UseInstrInfoNSZ.SignBit);
+
+    KnownFPClass NoUseInstrInfoNSZ = computeKnownFPClass(
+        A2, M->getDataLayout(), fcAllFlags, 0, nullptr, nullptr, nullptr,
+        nullptr, nullptr, /*UseInstrInfo=*/false);
+    EXPECT_EQ(SqrtMask, NoUseInstrInfoNSZ.KnownFPClasses);
+    EXPECT_EQ(std::nullopt, NoUseInstrInfoNSZ.SignBit);
+  }
+
+  {
+    KnownFPClass UseInstrInfoNoNan = computeKnownFPClass(
+        A3, M->getDataLayout(), fcAllFlags, 0, nullptr, nullptr, nullptr,
+        nullptr, nullptr, /*UseInstrInfo=*/true);
+    EXPECT_EQ(fcPositive | fcNegZero | fcQNan,
+              UseInstrInfoNoNan.KnownFPClasses);
+    EXPECT_EQ(std::nullopt, UseInstrInfoNoNan.SignBit);
+
+    KnownFPClass NoUseInstrInfoNoNan = computeKnownFPClass(
+        A3, M->getDataLayout(), fcAllFlags, 0, nullptr, nullptr, nullptr,
+        nullptr, nullptr, /*UseInstrInfo=*/false);
+    EXPECT_EQ(fcPositive | fcNegZero | fcQNan,
+              NoUseInstrInfoNoNan.KnownFPClasses);
+    EXPECT_EQ(std::nullopt, NoUseInstrInfoNoNan.SignBit);
+  }
+
+  {
+    KnownFPClass UseInstrInfoNSZNoNan = computeKnownFPClass(
+        A4, M->getDataLayout(), fcAllFlags, 0, nullptr, nullptr, nullptr,
+        nullptr, nullptr, /*UseInstrInfo=*/true);
+    EXPECT_EQ(fcPositive | fcQNan, UseInstrInfoNSZNoNan.KnownFPClasses);
+    EXPECT_EQ(false, UseInstrInfoNSZNoNan.SignBit);
+
+    KnownFPClass NoUseInstrInfoNSZNoNan = computeKnownFPClass(
+        A4, M->getDataLayout(), fcAllFlags, 0, nullptr, nullptr, nullptr,
+        nullptr, nullptr, /*UseInstrInfo=*/false);
+    EXPECT_EQ(fcPositive | fcNegZero | fcQNan,
+              NoUseInstrInfoNSZNoNan.KnownFPClasses);
+    EXPECT_EQ(std::nullopt, NoUseInstrInfoNSZNoNan.SignBit);
+  }
+}
+
 TEST_F(ValueTrackingTest, isNonZeroRecurrence) {
   parseAssembly(R"(
     define i1 @test(i8 %n, i8 %r) {
     entry:
       br label %loop
     loop:
       %p = phi i8 [ -1, %entry ], [ %next, %loop ]
       %next = add nsw i8 %p, -1