[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#4418573 <https://reviews.llvm.org/D149423#4418573>, @goldstein.w.n wrote:

> Is a third alternative to refactor the usage of `isSafeToExecuteWithDifferentOperands()` s.t instead of batching the analysis, it does the analysis/transform piece-meal? Or is that dramatically more work/have compile time/codegen impact? At the very least we could add a "IsBatchedAnalysis" flag (or API) and only apply the first two constraints if its set.

For some transforms, we need to ensure that e.g. all instructions in a block are speculatable, otherwise the transform is not legal. We can't really do that incrementally.

> Is there not a final concern that `isKnownNonZero` uses dominating conditions to determine zero/non-zero, so w.o a CxtI instruction we can't actually use `IsKnownNonZero` as the transform may change them i.e:
>
>   %c = icmp ule i32 %x, 1
>   br i1 %c, label %T, label %F
>   T:
>   ret i32 %y
>   F:
>   %r = udiv %y, %x
>   ret i32 %r
>
> we can't transform this into a `select` or something.

In this case we would be performing the query on `%x`, with the definition point of `%x` as the context, at which point there should be no dominating conditions for it. So I think there would be no problem.

> Think you could do the same with `assume` i.e:
>
>   %c = icmp ule i32 %z, 1
>   br i1 %c, label %T, label %F
>   T:
>   ret i32 %y
>   F:
>   %nz = icmp ne i32 %x, 0
>   call void @llvm.assume(i1 %nz)
>   %r = udiv %y, %x
>   ret i32 %r

The assume case may be problematic because we have limited backwards reasoning for assumes. Your example wouldn't hit this, but if you defined `%x` in the `F` block that could be a problem.

> For this, think we would need a flag is `IsKnownNonZero` to indicate we can't use assumes/dominating conditions to do `isKnowNonZero` (or just rely on `computeKnownBits.isNonZero()`)
>
>> In D149423#4402529 <https://reviews.llvm.org/D149423#4402529>, @goldstein.w.n wrote:
>>
>>> Is this still worth pursing?
>>
>> Not sure, to be honest. I liked this conceptually, but the problem turned out to be much more complicated than anticipated.
>
> I think given that we have an issue with loads a fix is needed either way. no?

It's two separate problems: The load issue is only about the isSafeToSpeculativelyExecute/isSafeToExecuteWithDifferentOperands split, it is not affected (as far as I can tell) by the trickier context instruction problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423