[PATCH] D138814: [InstCombine] Combine a/lshr of add -> (a + b < a)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/pr34349.ll:17
+; CHECK-NEXT:    [[ADD_NARROWED_OVERFLOW:%.*]] = icmp ult i7 [[TMP2]], [[TMP1]]
+; CHECK-NEXT:    [[V14:%.*]] = zext i1 [[ADD_NARROWED_OVERFLOW]] to i8
 ; CHECK-NEXT:    ret i8 [[V14]]
----------------
Pierre-vh wrote:
> nikic wrote:
> > This case doesn't look like a profitable transform. Do I understand correctly that the actual motivating case here is the case where the inputs are `zext`, and then this was later generalized based on reviewer feedback to use known bits instead?
> > 
> > For the zext case, this looks like an obviously desirable transform, but for the general case (where the truncs may not fold away) this is less clearly beneficial. I would personally restrict this to just zext unless we have specific motivation otherwise. (But I'm also not going to fight this if reviewers disagree.)
> I indeed did it using known bits after a comment from @spatel:
> 
> > In the earlier version(s) of this patch (is there a functional diff here from D107552?), there were questions about how it would interact with SCEV and/or vectorization.
> > 
> > If we're focusing on the overflow-only/minimal pattern, then I think we should structure the matching as a known-bits/demanded-bits problem. Ie, instead of zexts, we might have `and` masks. Instead of a shift at the end, that might also be a mask op.
> 
> For me it's fine to do it with just zext but then I'm afraid we'd miss some profitable cases
If we can get all of the motivating cases by matching zext directly, we can do that as a first step to reduce risk. Then, we can do the more general transform if needed as a second step.

IIUC, this means we could split off the ValueTracking part of the patch to an independent patch (add unit tests if there are no callers currently).

Also, please commit the baseline tests to main as a preliminary patch (D139011), so we can see current diffs (the bool math test diffs should be eliminated after 71df24dd39177ecfc440a0 ?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138814