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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

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

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

Going to post refactor patch that fixes load soonish (probably tomorrow), will run div patches through comile-time-tracker and maybe we will drop (or add flag for heavy analysis).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423