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

> 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 :)).

Took a look, think you are right.
Plan for this patch is to update the API by adding overload bool meaning `isSafeToSpeculativelyExecute(...., bool PreservesOperandCharacteristics = false)`.
If `PreservesOperandCharacteristics` is set, it means we the analysis promises the use will not modify the operands in a way that will invalidate the analysis (so only `trunc` if we known its still safe to speculatively execute). This should be usable to things like hoisting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149423