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