[PATCH] D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo

Wed Jul 28 12:33:08 PDT 2021

spatel added a comment.

In D106139#2910180 <https://reviews.llvm.org/D106139#2910180>, @dmgreen wrote:

>> In D106139#2909632 <https://reviews.llvm.org/D106139#2909632>, @lebedev.ri wrote:
>>
>>> In D106139#2909346 <https://reviews.llvm.org/D106139#2909346>, @abinavpp wrote:
>>>
>>>> @lebedev.ri Can you confirm the revision you require in this?
>>>
>>>
>>>
>>>> Is there an issue with the code in this patch and/or do you want us to move this to inst-combine?
>>>> I still agree with @arsenm that this sort of thing might need to be done in both the places, in which case I don't see any issue in the order in which we push for it.
>>>
>>> I agree that we might need to do this in both the places, emphasis is mine,
>>> but mainly this should be done in middle-end, and afterwards the back-end fold
>>> should be motivated by an end-to-end test showing the missed optimization.
>
> Do we usually create uadd.with.overflow in instcombine? I thought we moved all of that to CodeGenPrep as is was causing regressions otherwise.
> (I mean creating uadd.with.overflow from instructions that didn't start out as different add.with.overflow, generating them from plain llvm instructions. That might have been problems with the particular transform, but from what I remember instcombine was creating cross-basic-block overflow values, that the backend then couldn't handle very well. I'm not sure that was the exact reason for moving it though, I just remember codesize regressions).

This has gone back and forth. I don't think we have a consistent story yet. We clearly do create intrinsics in some cases like:
https://github.com/llvm/llvm-project/blob/0f4b41e038537ab2ab6fa2aa048e55c28a03ab68/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp#L1127
...but I have knowingly stayed away from some of those because of the backend problems that we've seen.
Side note: I was just looking at a buggy overflow fold in D106983 <https://reviews.llvm.org/D106983> where it's not clear to me that there is a universal correct answer.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106139