[PATCH] D135260: [AMDGPU][AMDGPULowerKernelAttributes] Use stripAndAccumulateConstantOffset

[Juan Manuel Martinez Caamaño via Phabricator via llvm-commits]

jmmartinez added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:84
+  return M.getFunction(Name);
+}
+
----------------
jdoerfert wrote:
> This function and uses are unrelated.
I'm sorry, I do not understand this remark. What do you mean?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:89
+    return L->isSimple() ? L : nullptr;
+  }
+
----------------
jdoerfert wrote:
> This does break with the "unique" property below. Two users, one simple one not, will result in a load. Is that expected?
It's ok if there is another non-simple `load`.

In fact, this pass could be modified to support multiple loads. My guess is that for simplicity only one is supported for the moment.

I renamed the function instead into `getUniqueSimpleLoadUser`


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:138
       continue;
+    }
 
----------------
jdoerfert wrote:
> I would use the `Attributor::checkForAllUses`, also unsure why the loads need to be unique. Depending on  the call `AAPointerInfo` would allow you to handle way more things without any of this manual tracing. Either seems to be a useful rewrite beyond the scope of this. Feel free to ignore for now.
I'm not familiar with it. I'm going to take a look at it. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135260