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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/lib/Basic/TargetInfo.cpp:405
+      const auto &OpenCLFeaturesMap = getSupportedOpenCLOpts();
+      Opts.OpenCLGenericAddressSpace = hasFeatureEnabled(
+          OpenCLFeaturesMap, "__opencl_c_generic_address_space");
----------------
azabaznov wrote:
> 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).
Apart from that `::adjust` was just introduced as a hack to mitigate the fact that the original clang flow was designed to set the types differently and now we are extending the hack to cover more areas.

Technically to adhere to clang flow we should probably set those options as target options but when we prototyped that it was too hard to condition the clang builtins on such options or something?

This way or another there seems to be more refactoring needed to preserve the flow fully. I think this at least deserves a `FIXME` comment to explain why we are ending up with the design that requires overriding the `LangOpts` here and also highlighting the fact that we have two places where it is set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103401