[llvm] 7ad3bb5 - Reapply [ValueLattice] Fix getCompare() for undef values

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2022-11-02T10:23:06+01:00
New Revision: 7ad3bb527e25eb0a9d147d2e93f9dca605c75688

URL: https://github.com/llvm/llvm-project/commit/7ad3bb527e25eb0a9d147d2e93f9dca605c75688
DIFF: https://github.com/llvm/llvm-project/commit/7ad3bb527e25eb0a9d147d2e93f9dca605c75688.diff

LOG: Reapply [ValueLattice] Fix getCompare() for undef values

Relative to the previous attempt, this also updates the
ValueLattice unit tests.

-----

Resolve the TODO about incorrect getCompare() behavior. This can
be made more precise (e.g. by materializing the undef value and
performing constant folding on it), but for now just return an
unknown result to fix the correctness issue.

This should be NFC in terms of user-visible behavior, because the
only user of this method (SCCP) was already guarding against
UndefValue results.

Added: 
    

Modified: 
    llvm/include/llvm/Analysis/ValueLattice.h
    llvm/lib/Transforms/Utils/SCCPSolver.cpp
    llvm/unittests/Analysis/ValueLatticeTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/Analysis/ValueLattice.h b/llvm/include/llvm/Analysis/ValueLattice.h
index bc6b279e9ed52..7fe45d9f4dc96 100644
--- a/llvm/include/llvm/Analysis/ValueLattice.h
+++ b/llvm/include/llvm/Analysis/ValueLattice.h
@@ -453,8 +453,14 @@ class ValueLatticeElement {
   /// evaluated.
   Constant *getCompare(CmpInst::Predicate Pred, Type *Ty,
                        const ValueLatticeElement &Other) const {
-    if (isUnknownOrUndef() || Other.isUnknownOrUndef())
-      return UndefValue::get(Ty);
+    // Not yet resolved.
+    if (isUnknown() || Other.isUnknown())
+      return nullptr;
+
+    // TODO: Can be made more precise, but always returning undef would be
+    // incorrect.
+    if (isUndef() || isUndef())
+      return nullptr;
 
     if (isConstant() && Other.isConstant())
       return ConstantExpr::getCompare(Pred, getConstant(), Other.getConstant());

diff  --git a/llvm/lib/Transforms/Utils/SCCPSolver.cpp b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
index a3455577a35c5..3d467f2fd68f9 100644
--- a/llvm/lib/Transforms/Utils/SCCPSolver.cpp
+++ b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
@@ -1044,9 +1044,6 @@ void SCCPInstVisitor::visitCmpInst(CmpInst &I) {
 
   Constant *C = V1State.getCompare(I.getPredicate(), I.getType(), V2State);
   if (C) {
-    // TODO: getCompare() currently has incorrect handling for unknown/undef.
-    if (isa<UndefValue>(C))
-      return;
     ValueLatticeElement CV;
     CV.markConstant(C);
     mergeInValue(&I, CV);

diff  --git a/llvm/unittests/Analysis/ValueLatticeTest.cpp b/llvm/unittests/Analysis/ValueLatticeTest.cpp
index b0b4b5e7bdc1d..5f5f249a7c7d3 100644
--- a/llvm/unittests/Analysis/ValueLatticeTest.cpp
+++ b/llvm/unittests/Analysis/ValueLatticeTest.cpp
@@ -173,24 +173,25 @@ TEST_F(ValueLatticeTest, getCompareUndef) {
   auto *I32Ty = IntegerType::get(Context, 32);
   auto *I1Ty = IntegerType::get(Context, 1);
 
+  // TODO: These results can be improved.
   auto LV1 = ValueLatticeElement::get(UndefValue::get(I32Ty));
   auto LV2 =
       ValueLatticeElement::getRange({APInt(32, 10, true), APInt(32, 20, true)});
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::ICMP_SLT, I1Ty, LV2)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::ICMP_SLE, I1Ty, LV2)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::ICMP_NE, I1Ty, LV2)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::ICMP_EQ, I1Ty, LV2)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::ICMP_SGE, I1Ty, LV2)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::ICMP_SGT, I1Ty, LV2)));
+  EXPECT_EQ(LV1.getCompare(CmpInst::ICMP_SLT, I1Ty, LV2), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::ICMP_SLE, I1Ty, LV2), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::ICMP_NE, I1Ty, LV2), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::ICMP_EQ, I1Ty, LV2), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::ICMP_SGE, I1Ty, LV2), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::ICMP_SGT, I1Ty, LV2), nullptr);
 
   auto *FloatTy = IntegerType::getFloatTy(Context);
   auto LV3 = ValueLatticeElement::get(ConstantFP::get(FloatTy, 1.0));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::FCMP_OEQ, I1Ty, LV3)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::FCMP_OGE, I1Ty, LV3)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::FCMP_OLE, I1Ty, LV3)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::FCMP_ONE, I1Ty, LV3)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::FCMP_OLT, I1Ty, LV3)));
-  EXPECT_TRUE(isa<UndefValue>(LV1.getCompare(CmpInst::FCMP_OGT, I1Ty, LV3)));
+  EXPECT_EQ(LV1.getCompare(CmpInst::FCMP_OEQ, I1Ty, LV3), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::FCMP_OGE, I1Ty, LV3), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::FCMP_OLE, I1Ty, LV3), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::FCMP_ONE, I1Ty, LV3), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::FCMP_OLT, I1Ty, LV3), nullptr);
+  EXPECT_EQ(LV1.getCompare(CmpInst::FCMP_OGT, I1Ty, LV3), nullptr);
 }
 
 } // end anonymous namespace