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

[Abinav Puthan Purayil via Phabricator via llvm-commits]

abinavpp marked 2 inline comments as done.
abinavpp 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)
----------------
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.


================
Comment at: llvm/test/CodeGen/AMDGPU/combine-srl-add.ll:13
+  ret i64 %add.c.shr
+}
----------------
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?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106139