[llvm] 051ec9f - [ValueTracking] Strengthen impliesPoison reasoning
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 19 11:00:18 PST 2021
On 1/19/21 9:04 AM, Nikita Popov via llvm-commits wrote:
> Author: Nikita Popov
> Date: 2021-01-19T18:04:23+01:00
> New Revision: 051ec9f5f43a83e23bd3e20e512fc5ec44c19850
>
> URL: https://github.com/llvm/llvm-project/commit/051ec9f5f43a83e23bd3e20e512fc5ec44c19850
> DIFF: https://github.com/llvm/llvm-project/commit/051ec9f5f43a83e23bd3e20e512fc5ec44c19850.diff
>
> LOG: [ValueTracking] Strengthen impliesPoison reasoning
>
> Split impliesPoison into two recursive walks, one over V, the
> other over ValAssumedPoison. This allows us to reason about poison
> implications in a number of additional cases that are important
> in practice. This is a generalized form of D94859, which handles
> the cmp to cmp implication in particular.
>
> Differential Revision: https://reviews.llvm.org/D94866
>
> Added:
>
>
> Modified:
> llvm/lib/Analysis/ValueTracking.cpp
> llvm/test/Transforms/InstCombine/select-and-or.ll
> llvm/unittests/Analysis/ValueTrackingTest.cpp
>
> Removed:
>
>
>
> ################################################################################
> diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
> index a9cef91e7316..7f8f101d42af 100644
> --- a/llvm/lib/Analysis/ValueTracking.cpp
> +++ b/llvm/lib/Analysis/ValueTracking.cpp
> @@ -4806,64 +4806,49 @@ bool llvm::canCreatePoison(const Operator *Op) {
> return ::canCreateUndefOrPoison(Op, /*PoisonOnly=*/true);
> }
>
> -bool llvm::impliesPoison(const Value *ValAssumedPoison, const Value *V) {
> - // Construct a set of values which are known to be poison from the knowledge
> - // that ValAssumedPoison is poison.
> - SmallPtrSet<const Value *, 4> PoisonValues;
> - PoisonValues.insert(ValAssumedPoison);
> - const Instruction *PoisonI = dyn_cast<Instruction>(ValAssumedPoison);
> - unsigned Depth = 0;
> - const unsigned MaxDepth = 2;
> -
> - while (PoisonI && Depth < MaxDepth) {
> - // We'd like to know whether an operand of PoisonI is also poison.
> - if (canCreatePoison(cast<Operator>(PoisonI)))
> - // PoisonI can be a poison-generating instruction, so don't look further
> - break;
> -
> - const Value *NextVal = nullptr;
> - bool MoreThanOneCandidate = false;
> - // See which operand can be poison
> - for (const auto &Op : PoisonI->operands()) {
> - if (!isGuaranteedNotToBeUndefOrPoison(Op.get())) {
> - // Op can be poison.
> - if (NextVal) {
> - // There is more than one operand that can make PoisonI poison.
> - MoreThanOneCandidate = true;
> - break;
> - }
> - NextVal = Op.get();
> - }
> - }
> +static bool directlyImpliesPoison(const Value *ValAssumedPoison,
> + const Value *V, unsigned Depth) {
> + if (ValAssumedPoison == V)
> + return true;
>
> - if (NextVal == nullptr) {
> - // All operands are non-poison, so PoisonI cannot be poison.
> - // Since assumption is false, return true
> - return true;
> - } else if (MoreThanOneCandidate)
> - break;
> + const unsigned MaxDepth = 2;
> + if (Depth >= MaxDepth)
> + return false;
>
> - Depth++;
> - PoisonValues.insert(NextVal);
> - PoisonI = dyn_cast<Instruction>(NextVal);
> + const auto *I = dyn_cast<Instruction>(V);
> + if (I && propagatesPoison(cast<Operator>(I))) {
> + return any_of(I->operands(), [=](const Value *Op) {
> + return directlyImpliesPoison(ValAssumedPoison, Op, Depth + 1);
> + });
> }
> + return false;
> +}
>
> - if (PoisonValues.contains(V))
> +static bool impliesPoison(const Value *ValAssumedPoison, const Value *V,
> + unsigned Depth) {
> + if (isGuaranteedNotToBeUndefOrPoison(ValAssumedPoison))
> return true;
>
> - // Let's look one level further, by seeing its arguments if I was an
> - // instruction.
> - // This happens when I is e.g. 'icmp X, const' where X is in PoisonValues.
> - const auto *I = dyn_cast<Instruction>(V);
> - if (I && propagatesPoison(cast<Operator>(I))) {
> - for (const auto &Op : I->operands())
> - if (PoisonValues.count(Op.get()))
> - return true;
> - }
> + if (directlyImpliesPoison(ValAssumedPoison, V, /* Depth */ 0))
> + return true;
This looks potentially very inefficient. You're doing a forward walk
each each step of the backwards walk. That's depth^2 work.
I believe you should be able to write this much more efficiently by
simply computing a forward set and a backwards set, then taking the
intersection. You only need to materialize one of course.
>
> + const unsigned MaxDepth = 2;
> + if (Depth >= MaxDepth)
> + return false;
> +
> + const auto *I = dyn_cast<Instruction>(ValAssumedPoison);
> + if (I && !canCreatePoison(cast<Operator>(I))) {
> + return all_of(I->operands(), [=](const Value *Op) {
> + return impliesPoison(Op, V, Depth + 1);
> + });
> + }
I think you may have a bug here. This seems to be assuming that all
instructions which can't create poison propagate poison from all
operands, which I don't believe to be true. I don't have a specific
counter example, but this looks suspect.
> return false;
> }
>
> +bool llvm::impliesPoison(const Value *ValAssumedPoison, const Value *V) {
> + return ::impliesPoison(ValAssumedPoison, V, /* Depth */ 0);
> +}
> +
> static bool programUndefinedIfUndefOrPoison(const Value *V,
> bool PoisonOnly);
>
>
> diff --git a/llvm/test/Transforms/InstCombine/select-and-or.ll b/llvm/test/Transforms/InstCombine/select-and-or.ll
> index 8681a7349ff9..f3de67649cb1 100644
> --- a/llvm/test/Transforms/InstCombine/select-and-or.ll
> +++ b/llvm/test/Transforms/InstCombine/select-and-or.ll
> @@ -148,10 +148,9 @@ define i1 @logical_or_noundef_a(i1 noundef %a, i1 %b) {
> }
>
> ; Noundef on false value allows conversion to or.
> -; TODO: impliesPoison doesn't handle this yet.
> define i1 @logical_or_noundef_b(i1 %a, i1 noundef %b) {
> ; CHECK-LABEL: @logical_or_noundef_b(
> -; CHECK-NEXT: [[RES:%.*]] = select i1 [[A:%.*]], i1 true, i1 [[B:%.*]]
> +; CHECK-NEXT: [[RES:%.*]] = or i1 [[A:%.*]], [[B:%.*]]
> ; CHECK-NEXT: ret i1 [[RES]]
> ;
> %res = select i1 %a, i1 true, i1 %b
> @@ -169,10 +168,9 @@ define i1 @logical_and_noundef_a(i1 noundef %a, i1 %b) {
> }
>
> ; Noundef on false value allows conversion to and.
> -; TODO: impliesPoison doesn't handle this yet.
> define i1 @logical_and_noundef_b(i1 %a, i1 noundef %b) {
> ; CHECK-LABEL: @logical_and_noundef_b(
> -; CHECK-NEXT: [[RES:%.*]] = select i1 [[A:%.*]], i1 [[B:%.*]], i1 false
> +; CHECK-NEXT: [[RES:%.*]] = and i1 [[A:%.*]], [[B:%.*]]
> ; CHECK-NEXT: ret i1 [[RES]]
> ;
> %res = select i1 %a, i1 %b, i1 false
>
> diff --git a/llvm/unittests/Analysis/ValueTrackingTest.cpp b/llvm/unittests/Analysis/ValueTrackingTest.cpp
> index 4b3b33b42625..f3240e0b4ff4 100644
> --- a/llvm/unittests/Analysis/ValueTrackingTest.cpp
> +++ b/llvm/unittests/Analysis/ValueTrackingTest.cpp
> @@ -748,6 +748,46 @@ TEST_F(ValueTrackingTest, impliesPoisonTest_AddNsw) {
> EXPECT_FALSE(impliesPoison(A2, A));
> }
>
> +TEST_F(ValueTrackingTest, impliesPoisonTest_Cmp) {
> + parseAssembly("define void @test(i32 %x, i32 %y, i1 %c) {\n"
> + " %A2 = icmp eq i32 %x, %y\n"
> + " %A0 = icmp ult i32 %x, %y\n"
> + " %A = or i1 %A0, %c\n"
> + " ret void\n"
> + "}");
> + EXPECT_TRUE(impliesPoison(A2, A));
> +}
> +
> +TEST_F(ValueTrackingTest, impliesPoisonTest_FCmpFMF) {
> + parseAssembly("define void @test(float %x, float %y, i1 %c) {\n"
> + " %A2 = fcmp nnan oeq float %x, %y\n"
> + " %A0 = fcmp olt float %x, %y\n"
> + " %A = or i1 %A0, %c\n"
> + " ret void\n"
> + "}");
> + EXPECT_FALSE(impliesPoison(A2, A));
> +}
> +
> +TEST_F(ValueTrackingTest, impliesPoisonTest_AddSubSameOps) {
> + parseAssembly("define void @test(i32 %x, i32 %y, i1 %c) {\n"
> + " %A2 = add i32 %x, %y\n"
> + " %A = sub i32 %x, %y\n"
> + " ret void\n"
> + "}");
> + EXPECT_TRUE(impliesPoison(A2, A));
> +}
> +
> +TEST_F(ValueTrackingTest, impliesPoisonTest_MaskCmp) {
> + parseAssembly("define void @test(i32 %x, i32 %y, i1 %c) {\n"
> + " %M2 = and i32 %x, 7\n"
> + " %A2 = icmp eq i32 %M2, 1\n"
> + " %M = and i32 %x, 15\n"
> + " %A = icmp eq i32 %M, 3\n"
> + " ret void\n"
> + "}");
> + EXPECT_TRUE(impliesPoison(A2, A));
> +}
> +
> TEST_F(ValueTrackingTest, ComputeNumSignBits_Shuffle_Pointers) {
> parseAssembly(
> "define <2 x i32*> @test(<2 x i32*> %x) {\n"
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list