[PATCH] D75401: [ValueTracking] Let isGuaranteedNotToBeUndefOrPoison look into branch conditions of dominating blocks' terminators

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune marked 4 inline comments as done.
aqjune added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4563
 
+  if (CxtI) {
+    // If V is used as a branch condition before reaching CxtI, V cannot be
----------------
reames wrote:
> jdoerfert wrote:
> > Style: early exit `!CtxI`.
> This should probably be inside programUndefinedIfFullPoison
I think the current one is more general than programUndefinedIfFullPoison(I) in that it takes only instructions as input whereas this code can take the case when V is a constant expression into account as well.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4579
+    if (!DT)
+      return hasCond(CxtI->getParent(), V);
+    else {
----------------
reames wrote:
> This is incorrect.  Consider:
> b = freeze a
> throw_if_a_would_be_poison()
> if (a == 5) {}
> 
> You can only use branches which either a) dominate the freeze's uses, or b) are guaranteed to execute if any of those uses execute.   (a) is a subcase of (b) which happens to be cheap to check.  Note that phrasing in terms of "uses" here, not the freeze def. 
You're right, so at least I should have seen idom(CxtI->getParent()), but not CxtI->getParent().
To clarify, CxtI is the location where we'd like to check whether V is poison or not, I left it as a comment at the declaration of isGuaranteedNotToBeUndefOrPoison. I used the text from `isSafeToSpeculativelyExecute()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75401