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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
