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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I still think there is something wrong if you need to consider the truncated and non truncated case. Are we missing an add width reduction case?



================
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:
> > 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
> We need to handle both N0.hasOneUse() case (i.e. only srl uses the add) and !N0.hasOneUse() iff the other uses are truncates to the type of the UADDO.add that we intend to create. If we do the combine rooted at trunc, we miss out the N0.hasOneUse() case. I think it makes more sense to do this rooted at srl since it's at srl's perspective we see that the add can be a uaddo. I'm not happy with the traversal of the uses of add since I like DAG-combine functions to only worry about the operands directions (i.e. towards the leaves) and not worry about the uses direction (i.e. towards the root), but to get the coverage I'm looking for, I can't see a better way to do this.
Is it actually profitable to handle the multiple use case?


================
Comment at: llvm/test/CodeGen/AMDGPU/combine-srl-add.ll:39
+entry:
+  %a.zext = sext i32 %a to i64
+  %b.zext = sext i32 %b to i64
----------------
Cases with both sext and zext would be good 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106139