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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106139