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

Fri Jul 17 16:29:13 PDT 2020

jdoerfert added inline comments.

================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4783
+
+  return false;
 }
----------------
nikic wrote:
> 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.
The implementation (=logic) to determine poison and undef should be at the same place. We can expose it via multiple functions but from the perspective of determining it the two are closely connection (IMHO).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84007