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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added a comment.

In D138814#3960318 <https://reviews.llvm.org/D138814#3960318>, @spatel wrote:

> 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.

I personally think we can create the add intrinsic if the add has more than one user, and the users are either the a/lshr or truncs (like we check now). I
In the end I'm not sure, to me it looks beneficial but I'll leave the final decision to people with more experience (cc @foad / @arsenm what do you think?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138814