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

> In D149423#4335035 <https://reviews.llvm.org/D149423#4335035>, @goldstein.w.n wrote:
>
>> In D149423#4334277 <https://reviews.llvm.org/D149423#4334277>, @StephenFan wrote:
>>
>>>> Short term fix is obviously revert (still building), but longer term
>>>> I guess we need an API for "is always speculatively executable"?
>>>> or something like that.
>>>> @nikic, any thoughts on how best to proceed in trying to safely
>>>> strengthen the `isSpeculativelyExecutable` case?
>>>
>>> I think strengthening the 'isSpeculativlyExecutable' is a bit conservative. I found that this bug is only exposed in CVP's narrowUDivOrURem, we can add some checks to avoid it.
>>
>> I tend to agree that this issue leans more on `narrowUDivOrURem` but are these implicit assumptions elsewhere in the project? If the common usage of `isSpeculativelyExecutable` has come to mean that its speculatively executable even if the non-const operands change then it is a bug in the change to `isSpeculativelyExecutable`, not with `narrowUDivOrURem`
>
> In GuardWidening there's a similar problem: when it first analyzes code, it calls `isSafeToSpeculativelyExecute` for a pair {instruction, insertion point} (here <https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/GuardWidening.cpp#L538>) and then it starts hoisting the instruction along with its operands, starting from the operands. While hoisting, it asserts that `isSafeToSpeculativelyExecute` should be true (here <https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/GuardWidening.cpp#L558>) since it should have been checked prior to hoisting and should still remain true. With this patch that assertion in some cases fails after the operands get hoisted. The return value of `isSafeToSpeculativelyExecute` changes because `isGuaranteedNotToBePoison` for some reason fails to prove that the denominator isn't poison, even though the hoisting had no impact on what values the operands can have. I understand that `isGuaranteedNotToBePoison` by its nature is always allowed to return false, so I don't think there's any problem with it.
> To illustrate my point, here's what the function looks like when GuardWidening calls `isSafeToSpeculativelyExecute('%sdiv', '%call')` for the first time:
>
>   define void @foo() {
>   bb:
>     br label %bb1
>   
>   bb1:                                              ; preds = %bb8, %bb
>     %phi = phi i32 [ 1, %bb ], [ %add9, %bb8 ]
>     %call = call i1 @llvm.experimental.widenable.condition()
>     br i1 %call, label %bb3, label %bb2
>   
>   bb2:                                              ; preds = %bb1
>     call void (...) @llvm.experimental.deoptimize.isVoid() [ "deopt"() ]
>     ret void
>   
>   bb3:                                              ; preds = %bb1
>     %zext = zext i32 %phi to i64
>     %sdiv = sdiv i64 1, %zext
>     %add = add nuw nsw i64 %zext, 1
>     %add4 = add nsw i64 %add, 1
>     %add5 = add nsw i64 %add4, %sdiv
>     %icmp = icmp sgt i64 %add5, 1
>     %call6 = call i1 @llvm.experimental.widenable.condition()
>     %and = and i1 %icmp, %call6
>     br i1 %and, label %bb8, label %bb7
>   
>   bb7:                                              ; preds = %bb3
>     call void (...) @llvm.experimental.deoptimize.isVoid() [ "deopt"() ]
>     ret void
>   
>   bb8:                                              ; preds = %bb3
>     %add9 = add nuw nsw i32 %phi, 1
>     br label %bb1
>   }
>
> And here's what the IR looks like when it calls `isSafeToSpeculativelyExecute('%sdiv', '%call')` the second time:
>
>   define void @foo() {
>   bb:
>     br label %bb1
>   
>   bb1:                                              ; preds = %bb8, %bb
>     %phi = phi i32 [ 1, %bb ], [ %add9, %bb8 ]
>     %zext = zext i32 %phi to i64
>     %add = add nuw nsw i64 %zext, 1
>     %add4 = add nsw i64 %add, 1
>     %call = call i1 @llvm.experimental.widenable.condition()
>     br i1 %call, label %bb3, label %bb2
>   
>   bb2:                                              ; preds = %bb1
>     call void (...) @llvm.experimental.deoptimize.isVoid() [ "deopt"() ]
>     ret void
>   
>   bb3:                                              ; preds = %bb1
>     %sdiv = sdiv i64 1, %zext
>     %add5 = add nsw i64 %add4, %sdiv
>     %icmp = icmp sgt i64 %add5, 1
>     %call6 = call i1 @llvm.experimental.widenable.condition()
>     %and = and i1 %icmp, %call6
>     br i1 %and, label %bb8, label %bb7
>   
>   bb7:                                              ; preds = %bb3
>     call void (...) @llvm.experimental.deoptimize.isVoid() [ "deopt"() ]
>     ret void
>   
>   bb8:                                              ; preds = %bb3
>     %add9 = add nuw nsw i32 %phi, 1
>     br label %bb1
>   }
>
> The only difference is that `%zext`, `%add` and `%add4` were hoisted to `bb1`.
>
> That problem isn't critical, and it probably can be resolved by removing the corresponding part of the assertion, but it demonstrates that some users indeed expect `isSafeToSpeculativelyExecute` to be resilient in face of some changes to the operands.

Yeah thats what I suspected. Maybe a new API is inorder? `isSafeToSpeculativelyExecuteConst` so that things like hoisting (which don't change the operands) can use the improve analysis but not things like GuardWidening or CVP. We could then replace where it makes sense rather than forcing the change to a 100 places which may rely on the _may change_ behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423