From 7ad3bb527e25eb0a9d147d2e93f9dca605c75688 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Tue, 1 Nov 2022 16:39:20 +0100 Subject: [PATCH] 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. --- llvm/include/llvm/Analysis/ValueLattice.h | 10 ++++++++-- llvm/lib/Transforms/Utils/SCCPSolver.cpp | 3 --- llvm/unittests/Analysis/ValueLatticeTest.cpp | 25 +++++++++++++------------ 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/llvm/include/llvm/Analysis/ValueLattice.h b/llvm/include/llvm/Analysis/ValueLattice.h index bc6b279..7fe45d9 100644 --- a/llvm/include/llvm/Analysis/ValueLattice.h +++ b/llvm/include/llvm/Analysis/ValueLattice.h @@ -453,8 +453,14 @@ public: /// 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 a345557..3d467f2 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(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 b0b4b5e..5f5f249 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(LV1.getCompare(CmpInst::ICMP_SLT, I1Ty, LV2))); - EXPECT_TRUE(isa(LV1.getCompare(CmpInst::ICMP_SLE, I1Ty, LV2))); - EXPECT_TRUE(isa(LV1.getCompare(CmpInst::ICMP_NE, I1Ty, LV2))); - EXPECT_TRUE(isa(LV1.getCompare(CmpInst::ICMP_EQ, I1Ty, LV2))); - EXPECT_TRUE(isa(LV1.getCompare(CmpInst::ICMP_SGE, I1Ty, LV2))); - EXPECT_TRUE(isa(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(LV1.getCompare(CmpInst::FCMP_OEQ, I1Ty, LV3))); - EXPECT_TRUE(isa(LV1.getCompare(CmpInst::FCMP_OGE, I1Ty, LV3))); - EXPECT_TRUE(isa(LV1.getCompare(CmpInst::FCMP_OLE, I1Ty, LV3))); - EXPECT_TRUE(isa(LV1.getCompare(CmpInst::FCMP_ONE, I1Ty, LV3))); - EXPECT_TRUE(isa(LV1.getCompare(CmpInst::FCMP_OLT, I1Ty, LV3))); - EXPECT_TRUE(isa(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 -- 2.7.4