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

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423