[PATCH] D149423: [ValueTracking] Use knownbits interface for determining if `div`/`rem` are safe to speculate

[luxufan via Phabricator via llvm-commits]

StephenFan added a comment.

In D149423#4416984 <https://reviews.llvm.org/D149423#4416984>, @nikic wrote:

> The second problem is related to the poison check and the context instruction. As far as I understand, the issue are callers of isSafeToSpeculativelyExecute() which don't speculate instructions one-by-one (as LICM does), but rather check a whole sequence for speculability first and then perform the actual transform. This ends up being problematic, because we perform poison queries on instructions in their previous positions, where the fact that they are passed to the division itself implies that they cannot be poison. So what we really want to determine here is "is this instruction non-poison at this context under the assumption that it has already been hoisted there", which is not what the existing APIs do.
>
> This problem is not as simple as not doing the programUndefinedIfUndefOrPoison() check, it can also occur in other contexts. Let's say we have a sequence like this:
>
>   %y = load i32, ptr %p, !noundef !{}, !range !{i32 1, i32 10}
>   %d = udiv i32 %x, %y
>
> Even if we ignore that `%y` is non-poison due to being used in the udiv, we also know that it is non-poison due to the `!noundef` metadata. However, that `!noundef` metadata would be dropped if the load were actually speculated. So in this case, the `udiv` is actually not safe to speculate, because `%y` may be poison after speculation.
>
> The two possible ways of solving this I can see are:
>
> 1. Add an extra argument to isGuaranteedNotToBeUndefOrPoison() that pretends instructions have been speculated.
> 2. Only support cases where the div arguments dominate the context instruction (if no context, that would effectively limit to constants/arguments). In that case the plain isGuaranteedNotToBeUndefOrPoison() should work fine.

Can we have the third solution: 
Continue or improving what we do in D150542 <https://reviews.llvm.org/D150542>. 
And as for

  %y = load i32, ptr %p, !noundef !{}, !range !{i32 1, i32 10}
  %d = udiv i32 %x, %y


When we check if `%y` is guaranteed not to be undef or poison, `%CtxI` argument should be respected. In other words, in `IsGuaranteedNotToBeUndefOrPoison`, if the load instruction's parent is not equal to `%CtxI`'s, then we should not reason about the result from the instruction's metadata. Since metadata may be dropped after moving to a different block. But I am also confused that if metadata like `!noundef` can be dropped randomly, why can we still reason about the result from metadata?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423