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

[Abinav Puthan Purayil via Phabricator via llvm-commits]

abinavpp marked an inline comment 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:
> 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?
I'm not sure if it will be profitable in all the cases, but we need to handle it to cover  https://github.com/RadeonOpenCompute/ROCm/issues/488


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106139