[PATCH] D149413: [ValueTracking] Add logic for `isKnownNonZero(usub.sat X, Y)`

Sat Apr 29 14:49:28 PDT 2023

goldstein.w.n added a comment.

In D149413#4307948 <https://reviews.llvm.org/D149413#4307948>, @nikic wrote:

> In D149413#4306606 <https://reviews.llvm.org/D149413#4306606>, @goldstein.w.n wrote:
>
>> In D149413#4304537 <https://reviews.llvm.org/D149413#4304537>, @nikic wrote:
>>
>>> This case looks unnecessary. If `%x >= %y` we will fold the usub.sat to `sub nuw %x, %y` and not hit this pattern.
>>
>> Most likely yeah. My feeling is it might exist outside of `InstCombine` and need to be analyzed. I.e if its analyzable in `InstSimplify`
>> it may enable further simplifications of patterns we don't want to have to duplicate in `InstCombine`.
>>
>> But if you're not convinced ill abandon.
>
> I'd generally say that we should only handle non-canonical patterns in ValueTracking/InstSimplify if there is some special motivation to do so, which probably doesn't exist here.
>
> I guess I'd be okay with handling it as a matter of "we handle all the other saturating intrinsics, why is this one missing?"

Yeah thats more my thinking.

> However, I'd prefer to see this implemented by extending isNonZeroSub() with a NUW flag that basically contains the logic you have here: https://alive2.llvm.org/ce/z/ZYDjCw That way the pattern will actually get handled after the usub.sat is replaced by sub nuw, and we're not just lying to ourselves about it working when it effectively doesn't.

Although its a pretty meaningless pattern to have for `sub`. `ugt` implies `ne` so the current `sub` pattern works just as well/better for `nuw` or no `nuw`.

I'll drop. I guess if you were to take my argument to the extreme we would have all the logic for reproducing `nsw`/`nuw` flags for add/mul/etc.... which is clearly
a bad idea.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149413