tra accepted this revision.
tra added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1845-1846
+ }
+ if (getCodeGenOpts().EmitOpenCLArgMetadata ||
+ getCodeGenOpts().HIPSaveKernelArgName)
Fn->setMetadata("kernel_arg_name",
----------------
b-sumner wrote:
> 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.
> 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?
`ShouldParseIf` appears to support expressions, so restricting it to hip + OCL
may be as simple as
```
ShouldParseIf<!strconcat(open_cl.KeyPath, "||", hip.KeyPath)>;
```
I'm not sure what to do about `-cl-kernel-arg-info` being in
`Group<opencl_Group>`. AFAICT, option currently may be only in one group. It's
not a problem for now as only OpenCL arg needs to be in a group, but that is a
point against merging the options. That makes my suggestion a wash -- a bit
better here, possibly worse there.
I withdraw the suggestion.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128022/new/
https://reviews.llvm.org/D128022
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
