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

[Artem Belevich via Phabricator via cfe-commits]

tra 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 {
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826