[llvm] 051ec9f - [ValueTracking] Strengthen impliesPoison reasoning
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 19 12:07:00 PST 2021
On Tue, Jan 19, 2021 at 8:00 PM Philip Reames <listmail at philipreames.com>
wrote:
>
> 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.
>
I'm not sure I understand what you have in mind here. The problem this is
solving is that if you have
%x = add i32 %a, %b
%y = add i32 %c, %d
%z = add i32 %x, %y
then poison is implied if either %z is used, or both %x and %y are used, or
all of %a, %b, %y are used, or all of %x, %c, %d are used or all of %a, %b,
%c, %d are used (but not any other combinations). And the same for the
other operand in the other direction. It's not clear to me how to do this
with a forward and backward set.
The complexity here should be about 2^(Depth1+Depth2). Typically we use 2^6
complexity cut-off for ValueTracking, this is using 2^(2+2) right now to be
conservative (this covers all the patterns we're interested in right now).
>
> > + 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.
>
I don't think so, but please double check my reasoning: Here we assume that
ValAssumedPoison is poison (if it isn't, then ex falso quodlibet).
ValAssumedPoison can be poison either because the instruction created
poison (which we have excluded), or because it propagated poison from an
operand. This reasoning does not require that poison is propagated from
*all* operands.
For example, consider poison implication from "select i1 %c, i32 %x, i32 0"
to "add i32 (zext i1 %c), %x". If we assume that the select is poison, then
we know that either %c is poison, or that %c is true and %x is poison. As
select cannot create poison, there are no other possibilities. As both %c
and %x are used (in a poison-propagating way) in the second expression, the
implication holds. This is despite the fact that select does not
unconditionally propagate poison. If %c were false and %x were not
propagated, then the select could not be poison in the first place, and
thus the implication also holds.
Does that make sense?
Regards,
Nikita
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210119/b13283bb/attachment.html>
More information about the llvm-commits
mailing list