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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D106139#2906928 <https://reviews.llvm.org/D106139#2906928>, @abinavpp wrote:

> In D106139#2904079 <https://reviews.llvm.org/D106139#2904079>, @lebedev.ri wrote:
>
>> In D106139#2904073 <https://reviews.llvm.org/D106139#2904073>, @abinavpp wrote:
>>
>>> In D106139#2904046 <https://reviews.llvm.org/D106139#2904046>, @lebedev.ri wrote:
>>>
>>>> Two comments:
>>>>
>>>> 1. the transform you want is https://alive2.llvm.org/ce/z/NFDE9C i.e. you need to start from `lshr i64 %z, 32`, match it's operands, and do *not* look at it's uses.
>>>
>>> We're not looking at lshr's uses here, but that of the add in lshr(add()). I would like this to cover truncUse() in AMDGPU/combine-srx-add.ll (g() in X86/addcarry.ll). I'm not sure if we can do that without looking at the add's uses.
>>>
>>> In D106139#2904046 <https://reviews.llvm.org/D106139#2904046>, @lebedev.ri wrote:
>>>
>>>> 2. This is really something that should go into instcombine
>>>
>>> @foad mentioned the same. While I understand that target-specific combines should go to lib/target/<target> with setTargetDAGCombine(), I'm not able to determine if the target independent ones should go to target independent version of inst-combine, DAG-combine, or both. I understand that keeping at both has the advantage of better coverage since inst-combine might catch the pattern that DAG-combine might not be able to catch or vice-versa. I'm not able to see any motivation in *moving* this to inst-combine, but I do see a motivation in *copying* this to inst-combine.
>>
>> This is a very straight-forward fold for a simple IR pattern. There is absolutely nothing target-specific there.
>> Unless said pattern can be produced by the backend lowering, it should be done in the middle-end, not here.
>
> The DAG-combine we're doing here is target independent. I'm still not sure about moving this to inst-combine.



> We can move and/or copy this to inst-combine once we see cases where the DAG-combine approach is underperforming. I hope that's OK.

This is backwards.
*If* the pattern in question can somehow appear after the last time instcombine pass run in pipeline,
then it's justified to have a similar DAG combine for it. But in general it should be in instcombine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106139