[PATCH] D106139: [AMDGPU] Combine srl of add that intends to get the carry of the add as addcarry

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:3285-3286
+
+  // Make sure that the users of `N0` is not expecting the zero-extended type
+  for (SDNode *U : N0->uses()) {
+    if (U == N)
----------------
abinavpp wrote:
> arsenm wrote:
> > I don't understand why you are looking at the uses, you probably should just  skip this if !N0.hasOnEUse
> The add I'm handling in https://github.com/RadeonOpenCompute/ROCm/issues/488 is used by a truncate, so we need to check for it to improve coverage. https://alive2.llvm.org/ce/z/Vv4DSr is what I'm trying to cover here.
If the transform can only be done from a trunc root, the combine should start by looking at the trunc, not the add


================
Comment at: llvm/test/CodeGen/AMDGPU/combine-srl-add.ll:13
+  ret i64 %add.c.shr
+}
----------------
abinavpp wrote:
> arsenm wrote:
> > This could use a lot more tests, like making sure the number of bits in the shift match, and that the sources are zexts. Also what if tere are multiple uses of the add?
> > 
> > Also should cover sub, and stress both vector and scalar versions. Does this also work with sext if the shift is ashr?
> I would like this patch to just handle (srl (add (zext))) kinds. I'll improve the coverage for ashr, vector, sub etc. in different patches.
> 
> 
> > like making sure the number of bits in the shift match, and that the sources are zexts. Also what if tere are multiple uses of the add?
> You mean like adding test-cases that shows no transformation for unfavourable shift value, (srl (add (sext))) kinds?
> 
> 
Yes, negative tests where this transform is invalid


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106139