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

[Michael Liao via Phabricator via cfe-commits]

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


================
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:
> hliao wrote:
> > 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.
> You may not need it, ever and it would be easy to add, but I'll leave it up to you.
> 
> If you do want to keep them as parameters you may want to consider renaming them to FromAS/ToAS.
> There's nothing in the code that has anything to do with whether they are for generic/specific address space and the function name does not indicate the direction of coercion between them. It's very easy to pass them in the wrong order and not notice it. Making them local variables would avoid it. Giving names some sort of 'directionality' would at least give user a hint what goes where, even if it would not prevent making the error.
> 
>From the target device side, we have generic and global addresses. But, at the language level, we have `opencl_global` and `cuda_device`. Even though they map into the same address space, it would be very confusing if they are misused to initialize that address space numbers. That's why the original helper makes more sense to me and makes the code more readable. Anyway, I change the parameter names to give a clear direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826