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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp:303
         for (Value *V : I.operands())
           Checks.push_back(getPoisonFor(ValToPoison, V));
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111643