[PATCH] D69826: [hip] Enable pointer argument lowering through coercing type.

[Michael Liao via Phabricator via cfe-commits]

hliao marked 3 inline comments as done.
hliao added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7719
+      // Arrary types.
+      if (auto ATy = dyn_cast<llvm::ArrayType>(Ty)) {
+        auto T = ATy->getElementType();
----------------
hliao wrote:
> hliao wrote:
> > arsenm wrote:
> > > No tests with arrays or structs?
> > > 
> > > It's also not immediately obvious to me that this optimization is still valid if the pointer is buried in a struct
> > the original generic kernel pointer promotion to a global one only handles the pointer directly passed. From a critical workload, I found quite a few cases where the global pointers are passed through a by-val struct. We didn't handle that yet. With this case, we could start to handle that.
> struct tests are added. From test cases, it seems to me that arry is not passed by value. I need to double-confirm.
a test case for arrary types is added.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7689
+  // Coerce HIP pointer arguments from generic pointers to global ones.
+  llvm::Type *coerce(llvm::Type *Ty, unsigned DefaultAS,
+                     unsigned GlobalAS) const {
----------------
tra wrote:
> Now it could use a more descriptive name, too. :-)
> 
> You can now also make DefaultAS/GlobalAS into local variables as you have access to `getContext()` here.
name is changed but I want to leave `DefaultAS` and `GlobalAS` as parameters as they may vary from HIP to OpenCL and different targets. Even though it may be rare case, I want to avoid careless errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826