[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#4416984 <https://reviews.llvm.org/D149423#4416984>, @nikic wrote:

> Okay, so as I understand it, this patch ran into two separate problems:
>
> The first is that isSafeToSpeculativeExecute() is used for two different purposes: One is to speculate the instruction (i.e. execute it where it may not have previously been executed, keeping the same arguments), and the other is to execute the instruction at the same position but with different operands (with the implicit assumption that constant operands stay the same). Previously, the same function worked for both of those, but this is no longer the case if we analyze variables.
>
> Actually, I think this is not quite true and we have a pre-existing issue for loads. Consider this example: https://alive2.llvm.org/ce/z/weHUyL If there were a control flow exit between the load and the select, this might speculate a trapping load.
>
> With this in mind, this is clearly something that needs to be solved. My preference would to provide two separate functions (with a shared implementation) for the two different use cases, rather than specifying a flag everywhere. For example, one stays isSafeToSpeculativelyExecute() and the other becomes isSafeToExecuteWithDifferentOperands(). I think we can do this independently of the div change, with the load case as the motivating example.
>
> 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.

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.

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.

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

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423