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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D138814#3960260 <https://reviews.llvm.org/D138814#3960260>, @Pierre-vh wrote:

> In D138814#3960221 <https://reviews.llvm.org/D138814#3960221>, @spatel wrote:
>
>> In D138814#3959942 <https://reviews.llvm.org/D138814#3959942>, @foad wrote:
>>
>>> I don't think there is any requirement for the wider type to be exactly double the narrower type.
>>
>> That's correct:
>> https://alive2.llvm.org/ce/z/iLVIgn
>>
>> So this patch/tests are too narrow as-is. It should be checking something like "if we only demand the top N bits of an add, and the add operands are known zero in those top N bits, then fold the add into an overflow check."
>>
>> Also, canonicalizing to the add intrinsic if we're not using the add part of the result seems like the wrong direction. I can't tell from the larger test what we're expecting to happen. Please pre-commit the baseline tests, so we can see the diffs.
>
> Will update the combine & add a base test diff.
>
> Do you mean we shouldn't do the combine if the Add has only one use?

I think it's the inverse - if the add has only one use, then fold to "not+icmp+zext":
https://github.com/llvm/llvm-project/issues/59232

If the add has >1 use, then I'm not sure what we want to happen. In the general form, we have something like this:
https://alive2.llvm.org/ce/z/sW5BME
...so what other pieces of the pattern need to be there to justify creating the add intrinsic? We're in target-independent InstCombine here, so we don't usually want to end up with more instructions than we started with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138814