[PATCH] D111643: [ValueTracking] Let propgatesPoison consider single poison operand.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D111643#3058773 <https://reviews.llvm.org/D111643#3058773>, @reames wrote:

> This looks pretty reasonable to me overall.  A couple high level suggestions:
>
> - I second the suggestion of taking the new operand as a Use.

That should be doable with adjusting the callers a bit. I'll update the patch.

> - Framing wise, I suggest making it clear that we're restricting the set of operands known to be poison.  And that the nullptr value has no restriction.  I found your comment and patch framing difficult to decipher.  I had to work back to what you meant from the code.

I'll try to make it a bit clearer. I might be mis-interpreting the exact meaning of restricting, but I am not sure if that's entirely accurate. With the nullptr value, we known that at least one of the operands is poison, but we do not know which; if a value is passed, we know that exactly this value is poison.

Extending it to an ArrayRef would mean the following I think: if it is empty, at least one operand is poison, but we do not know which. Otherwise we know that all values in the ArrayRef are poison. I think the ArrayRef could also be used to communicate the fact that both options of a select are poison, hence the select itself is poison.

In D111643#3058379 <https://reviews.llvm.org/D111643#3058379>, @aqjune wrote:

> The change looks great to me.
>
>> Alive2 proofs for the changed instcombine tests:
>
> Were the InstCombine test changes supposed to be included in this patch?

Argh, originally they were, but it turns out there was a problem with early iterations that made us transform the test cases. While it was correct for the test cases, it was not correct in general unfortunately. I added a SCEV IR test in 4cd6cc64edb3833327601fb9eb2b5a9b6fb2bb7f <https://reviews.llvm.org/rG4cd6cc64edb3833327601fb9eb2b5a9b6fb2bb7f>



================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:609
+  bool propagatesPoison(const Operator *I,
+                        const Value *ValAssumedPoison = nullptr);
 
----------------
nikic wrote:
> Can we make this function accept (only) a `Use`?
I think that should be possible, it just requires updating a few of the users.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111643/new/

https://reviews.llvm.org/D111643