[PATCH] D103401: [OpenCL] Add support of __opencl_c_generic_address_space feature macro

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added inline comments.


================
Comment at: clang/lib/Basic/TargetInfo.cpp:405
+      const auto &OpenCLFeaturesMap = getSupportedOpenCLOpts();
+      Opts.OpenCLGenericAddressSpace = hasFeatureEnabled(
+          OpenCLFeaturesMap, "__opencl_c_generic_address_space");
----------------
Anastasia wrote:
> azabaznov wrote:
> > Anastasia wrote:
> > > svenvh wrote:
> > > > This means we now have two separate places that set `OpenCLGenericAddressSpace`, the other place being in `CompilerInvocation::setLangDefaults()`.  That feels like a maintenance hazard.  Do you think it makes sense to set this field in one single place instead?
> > > I think we should try to set it in `CompilerInvocation.cpp` directly, we should be able to query `TargetOptions` there. Although that place is expected to be for the language-specific defaults but we broke the standard flow by having the language mode controlled by the target settings anyway.
> > > 
> > > I can't remember though why we have decided to add dedicated `LangOpts` entries for generic address space instead of just using `OpenCLOptions` from `Sema`? I think it simplifies the handling of some builtin functions?
> > > This means we now have two separate places that set OpenCLGenericAddressSpace, the other place being in CompilerInvocation::setLangDefaults(). That feels like a maintenance hazard. Do you think it makes sense to set this field in one single place instead?
> > 
> > >I think we should try to set it in CompilerInvocation.cpp directly, we should be able to query TargetOptions there. 
> > 
> > I don't think that we are able to access target options at that stage without modifying current interfaces.  `CompilerInvocation::setLangDefaults()` is a static member function.
> > 
> > > I can't remember though why we have decided to add dedicated LangOpts entries for generic address space instead of just using OpenCLOptions from Sema? I think it simplifies the handling of some builtin functions?
> > 
> > That's correct. Also, the idea was to reuse generic keyword in other languages.
> > I don't think that we are able to access target options at that stage without modifying current interfaces. CompilerInvocation::setLangDefaults() is a static member function.
> 
> I wonder if the function is static due to an old interface or something because it seems to be only called from `CompilerInvocation::ParseLangArgs` which isn't a static member as far as I can see. I wonder if it should just become a non-static member instead?
> 
Actually `CompilerInvocation::ParseLangArgs` is static as well. Anyway, I think this change would be really impactful. I believe `::adjust` is a prefect place to coordinate language options with target information (at least for now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103401