[PATCH] D84007: [ValueTracking] Change canCreatePoison to take Operator, add canCreateUndef

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4783
+
+  return false;
 }
----------------
jdoerfert wrote:
> nikic wrote:
> > jdoerfert wrote:
> > > nikic wrote:
> > > > jdoerfert wrote:
> > > > > This seems wrong. A load of an uninitialized location should produce `undef` for example, shouldn't it? I fear there are way more things that can produce `undef`. The default should be true here.
> > > > Returning true here would make this function always return true :) If we drop the "undef only" mode, then returning false should be fine, as this function only needs to deal with cases where undef is possible but poison is not.
> > > What about my load example? poison is not possible but undef is, returning false still seems wrong, or maybe I'm just confused..
> > Loads can also return poison, and canCreatePoison() will return true for them. (Having load/store implicitly filter poison would prevent mem2reg/SROA.)
> Loads *can* return poison. Some are known *not to* but still *can* return `undef`. Are we sure this implicit link between the two procedures is wise? I mean, even if the situation I describe doesn't happen now, how do you prevent someone from making `canCreatePoison` smarter and introducing a bug here?
Okay, I see your concern now. In that case I would suggest combining these into a single canCreateUndefOrPoison() function with a PoisonOnly flag (for the PoisonChecking consumer). That should make it obvious that these need to be considered as a unit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84007