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

[Abinav Puthan Purayil via Phabricator via llvm-commits]

abinavpp added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106139