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

Sat Apr 29 12:51:43 PDT 2023

nikic added a comment.

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?"

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149413