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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp:303
         for (Value *V : I.operands())
           Checks.push_back(getPoisonFor(ValToPoison, V));
 
----------------
nikic wrote:
> fhahn wrote:
> > nikic wrote:
> > > This doesn't look quite right to me, shouldn't this only push a check specifically for poison-propagating operands, rather than for all operands if at least one propagates?
> > Agreed, this is more obvious now, but it should retain the original behavior, right? I'd prefer to not pull in a separate functional improvement in this patch if possible.
> If I understand correctly, `ValToPoison` computes "this value is definitely poison". Previously, the `propagatesPoison()` check told you that any poison operand would make the result also poison, so simply or'ing together ValToPoison of all operands was correct. Now that only some operands may propagate, it's no longer correct. For the select case, we should only be using the condition operand, while as written it would or all three operands.
> 
> I think your code would be correct if you replaced any_of with all_of, but only picking the right operands seems like it would be cleaner and simpler.
Updated to check for each operand individually. Added some extra tests in 869f60ffa13bdcb02


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111643