[PATCH] D138814: [InstCombine] Combine a/lshr of add -> uadd.with.overflow

Mon Nov 28 09:07:39 PST 2022

spatel added a comment.

In D138814#3953839 <https://reviews.llvm.org/D138814#3953839>, @foad wrote:

> In D138814#3953742 <https://reviews.llvm.org/D138814#3953742>, @spatel wrote:
>
>> In D138814#3953729 <https://reviews.llvm.org/D138814#3953729>, @foad wrote:
>>
>>>> What do you think about producing the sequence with 'not' + 'icmp' instead of the uadd + extract?
>>>
>>> The point about using uadd.with.overflow is that it also gives you the truncated 32 bit result of the add.
>>
>> If we're not using that value (as shown in the first two tests), then do we still we want to canonicalize to an overflow intrinsic? That implies that we also need to transform the "not+icmp" pattern into "uadd+extract" to be consistent.
>
> Oh, I see. In that case I guess the "not" form is reasonable. Hopefully value range tracking understands it.

Right - I don't know which form is going to get us to the ideal final output (might be different for different patterns). 
In the earlier version(s) of this patch (is there a functional diff here from D107552 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138814