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

[luxufan via Phabricator via llvm-commits]

StephenFan 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
>   }

For this IR, I think  `isSafeToSpeculativelyExecute('%sdiv', '%call')` should return false. I implemented a patch D150542 <https://reviews.llvm.org/D150542> to give my opinion (may be totally wrong :)).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423