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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:607
+  /// poison from non-undef & non-poison operands.
+  bool canCreateUndef(const Operator *Op, bool ConsiderPoisonAsUndef);
 
----------------
jdoerfert wrote:
> Drive by: Now that I see this, I would prefer the public API to have two functions. I'm fine if others disagree but for the users it seems more natural to have `canCreateUndef` and `canCreateUndefOrPoison`, (and maybe `canCreatePoison`).
Same here. I would prefer there to only be canCreatePoison and canCreatePoisonOrUndef. Just canCreateUndef by itself would be a footgun, because poison relaxes to undef. I don't see an immediate use for a "can create undef excluding undef that was originally poison" query.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4783
+
+  return false;
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84007