[PATCH] D128022: [HIP] add -fhip-kernel-arg-name

[Brian Sumner via Phabricator via cfe-commits]

b-sumner added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1845-1846
+  }
+  if (getCodeGenOpts().EmitOpenCLArgMetadata ||
+      getCodeGenOpts().HIPSaveKernelArgName)
     Fn->setMetadata("kernel_arg_name",
----------------
yaxunl wrote:
> tra wrote:
> > yaxunl wrote:
> > > yaxunl wrote:
> > > > tra wrote:
> > > > > tra wrote:
> > > > > > Should we consolidate both options into `-fkernel-arg-info` and make `-cl-kernel-arg-info` an alias to it?
> > > > > Also, this check is odd. For some reason only *arg name*  metadata is set conditionally, but for whatever reason OpenCL sets other arg metadata unconditionally.
> > > > > 
> > > > > Now I'm really curious what's so special about "kernel_arg_name" vs the other arg metadata.
> > > > > Should we consolidate both options into `-fkernel-arg-info` and make `-cl-kernel-arg-info` an alias to it?
> > > > 
> > > > -cl-kernel-arg-info is an OpenCL option defined in OpenCL spec, therefore is made OpenCL only option. It would be confusing to allow it with other languages.
> > > > Also, this check is odd. For some reason only *arg name*  metadata is set conditionally, but for whatever reason OpenCL sets other arg metadata unconditionally.
> > > > 
> > > > Now I'm really curious what's so special about "kernel_arg_name" vs the other arg metadata.
> > > 
> > > The other metadata are mandatory because they are necessary for OpenCL runtime to set kernel argument.
> > > 
> > > The kernel argument name is emitted only with -cl-kernel-arg-info since it is only used to support clGetKernelArgInfo which requires -cl-kernel-arg-info to work.
> > >  It would be confusing to allow it with other languages.
> > 
> > On the other hand having two options that control exactly the same functionality also looks odd to me.
> > 
> > The way I see it is that an entity may have more than one name. OpenCL standard requires that that particular functionality must be enabled via `-cl-kernel-arg-info` and I'm not proposing to change that. The OpenCL standard has no say whether that flag may have a different name in addition to the standard-required one.
> > 
> > This is similar to how over time we've been transitioning what used to be CUDA-only options into their generic `-gpu` and `-offload` variants, only in this case OpenCL functionality becomes useful outside of OpenCL.
> > 
> > >  It would be confusing to allow it with other languages.
> > 
> > On the other hand having two options that control exactly the same functionality also looks odd to me.
> > 
> > The way I see it is that an entity may have more than one name. OpenCL standard requires that that particular functionality must be enabled via `-cl-kernel-arg-info` and I'm not proposing to change that. The OpenCL standard has no say whether that flag may have a different name in addition to the standard-required one.
> > 
> > This is similar to how over time we've been transitioning what used to be CUDA-only options into their generic `-gpu` and `-offload` variants, only in this case OpenCL functionality becomes useful outside of OpenCL.
> > 
> 
> If we introduce a generic option -fkernel-arg-info and make -cl-kernel-arg-info alias to it, do we want to make it available to all languages or limit it to HIP and OpenCL only?
> > >  It would be confusing to allow it with other languages.
> > 
> > On the other hand having two options that control exactly the same functionality also looks odd to me.
> > 
> > The way I see it is that an entity may have more than one name. OpenCL standard requires that that particular functionality must be enabled via `-cl-kernel-arg-info` and I'm not proposing to change that. The OpenCL standard has no say whether that flag may have a different name in addition to the standard-required one.
> > 
> > This is similar to how over time we've been transitioning what used to be CUDA-only options into their generic `-gpu` and `-offload` variants, only in this case OpenCL functionality becomes useful outside of OpenCL.
> > 
> 
> If we introduce a generic option -fkernel-arg-info and make -cl-kernel-arg-info alias to it, do we want to make it available to all languages or limit it to HIP and OpenCL only?

I think that would be fine. 


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

https://reviews.llvm.org/D128022