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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D149423#4420186 <https://reviews.llvm.org/D149423#4420186>, @StephenFan wrote:

> 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?

I don't think this is appropriate, because it overloads the meaning of `%CtxI`, between "not poison at this point" and "not poison if hoisted to this point". Generally speaking, specifying a context instruction should only improve ValueTracking results, not make the more conservative.

We can do what you suggest in general, but it should not use the existing `CtxI` argument.

However, together with the additional concern around context-sensitive facts (like assumes) that @goldstein.w.n raised, I think that going for "require div operands to dominate the context instruction" is going to be the most robust solution.

An unrelated thought I had is that speculating these divs might run into a cost issue: We currently only speculate divs with constant divisor, which also ensures that the div will be expanded to arithmetic as a side-effect, so we never actually end up speculating real division instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423