azabaznov added inline comments.
================ Comment at: clang/include/clang/Basic/OpenCLExtensions.def:17 // If the extensions are to be enumerated without the supported OpenCL version, -// define OPENCLEXT(ext) where ext is the name of the extension. +// define OPENCLEXTNAME(ext) where ext is the name of the extension. // ---------------- Anastasia wrote: > I guess you mean that this extension is not against a specific version i.e. > applies to all versions? Yeah, this is needed to enumerate all extensions (as it was earlier). I only changed the naming of macro in this patch, I'll adjust the comment. ================ Comment at: clang/include/clang/Basic/OpenCLExtensions.def:79 +OPENCL_EXTENSION(cl_khr_subgroups, 200) +OPENCL_EXTENSION(cl_khr_subgroup_extended_types, 200) +OPENCL_EXTENSION(cl_khr_subgroup_non_uniform_vote, 200) ---------------- Anastasia wrote: > Is this code accidental? The following extensions `cl_khr_subgroup_*` were > recently removed from this file. > > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/OpenCLExtensions.def Yes, thanks for noticing. This got here accidentally after rebasing. ================ Comment at: clang/include/clang/Basic/OpenCLOptions.h:55 +// OpenCL C version mask +static inline bool OpenCLVersionIsContainedInMask(const LangOptions &LO, + unsigned Mask) { ---------------- Anastasia wrote: > A small renaming > `OpenCLVersionIsContainedInMask` -> `isOpenCLVersionContainedInMask` Sure, will change. ================ Comment at: clang/include/clang/Basic/OpenCLOptions.h:62 - /// Check if \c Ext is supported as an (optional) OpenCL core features for - /// the given OpenCL version. - /// - /// \param Ext - Extension to look up. - /// \param LO - \c LangOptions specifying the OpenCL version. - /// \returns \c true if \c Ext is known and supported, \c false otherwise. - bool isSupportedCore(llvm::StringRef Ext, const LangOptions &LO) const { - auto E = OptMap.find(Ext); - if (E == OptMap.end()) { - return false; - } - // In C++ mode all extensions should work at least as in v2.0. - auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion; - auto I = E->getValue(); - return I.Supported && I.Avail <= CLVer && I.Core != ~0U && CLVer >= I.Core; - } +struct OpenCLOptionInfo { + // Option starts to be available in this OpenCL version ---------------- Anastasia wrote: > I think it would be better to keep it in `OpenCLOptions`, as we don't intend > this to be used stand-alone also considering that it's a `struct`? I > understand that this is now being used in Targets too but that use should > hopefully be eliminated in the future. This check is needed to check availability of extensions (see comment in `Targets.cpp`). Or you mean to define it as a nested class in `OpenCLOptions`? ================ Comment at: clang/lib/Basic/TargetInfo.cpp:360 + // Set core features based on OpenCL version + for (auto CoreExt : clang::getCoreFeatures(Opts)) + getTargetOpts().OpenCLFeaturesMap[CoreExt] = true; ---------------- Anastasia wrote: > I still think the target map should be immutable and especially we should not > change it silently based on the language compiled even if we have done it > before but that caused incorrect behavior i.e. successfully compiling for the > architectures that didn't support the features. > > If I look at existing targets they already set most of the core features > apart from 3d image writes. Perhaps it is reasonable to just drop this code? > I don't think it makes the issue worse, in fact, I think it will make the > behavior slightly better because now a diagnostic will occur if there is an > attempt to use the unsupported feature although the diagnostic won't be the > optimal one. After all it will still remain the responsibility of the user > to get the right combination of a language version and a target. > > It would be reasonable however to introduce a diagnostic that would report a > mismatch between the language version and the hardware support available. We > report similar diagnostics in `CompilerInvocation` already. But I don't think > we have to do it in this patch because it doesn't introduce any regression. > We already have a bug although the behavior of this bug will change. And > perhaps if we add `OpenCLOptions` as a part of `LangOpts` at some point this > will become straightforward to diagnose. However, I suggest we add > information about this issue in a FIXME or perhaps this deserves a clang bug! > I still think the target map should be immutable and especially we should not > change it silently based on the language compiled I'm confused. I think we have agreed to unconditionally support core features for a specific language version. Did I miss something? > successfully compiling for the architectures that didn't support the features. I like idea providing diagnostics in that case. Something like: "Warning: r600 target doesn't support cl_khr_3d_image_writes which is core in OpenCL C 2.0, consider using OpenCL C 3.0". I also think this should be done in a separate commit. > If I look at existing targets they already set most of the core features > apart from 3d image writes. Perhaps it is reasonable to just drop this code? Oh, I haven't noticed that target set core features. For example //cl_khr_global_int32_base_atomics// is being set by NVPTX and AMDGPU, so I agree that this should be removed from target settings. ================ Comment at: clang/lib/Basic/Targets.cpp:728 + // Check if extension is supported by target or is a core feature + auto It = getTargetOpts().OpenCLFeaturesMap.find(Name); + if ((It != getTargetOpts().OpenCLFeaturesMap.end()) && It->getValue() && ---------------- Anastasia wrote: > I still think it would be better if we just iterate over the elements in this > map? We should keep the includes of `clang/Basic/OpenCLExtensions.def ` to a > minimum. This is to check if extension/feature is available (by calling `OpenCLOptionInfo::isAvailableIn(LangOpts)`). For example, //cl_khr_srgb_image_writes// (which is available only in OpenCL C 2.0) can be supported via command line for OpenCL C 1.2 which is erroneous. Information about availability of an extension is stored in `OpenCLExtensions.def`. ================ Comment at: clang/lib/Basic/Targets.cpp:730 + if ((It != getTargetOpts().OpenCLFeaturesMap.end()) && It->getValue() && + OpenCLOptionInfo(AvailVer, CoreVersions, OptionalVersions) + .isAvailableIn(Opts)) ---------------- Anastasia wrote: > I think this defines should belong elsewhere, this will eliminate the need to > create the temporary object `OpenCLOptionInfo`. But I appreciate there is > only limited amount of refactoring we can do in one patch. > > How about we at least add a `FIXME` explaining that some further refactoring > is needed to move the macro definition with the rest of macros from > `LangOpts`. > > > this will eliminate the need to create the temporary object OpenCLOptionInfo This temporary object is needed to check availability of extension (see comment above). ================ Comment at: clang/lib/Basic/Targets.cpp:725 + if ((It != OpenCLFeaturesMap.end() && It->getValue()) || + OpenCLOptionInfo(AvailVer, CoreVersions, OptionalVersions) + .IsCoreIn(Opts)) ---------------- Anastasia wrote: > azabaznov wrote: > > Anastasia wrote: > > > I think we should find different place for those now. > > > > > > Ideally, we should iterate though the map in `OpenCLOptions` and set the > > > define for the supported ones. > > > > > > I suggest we move this into `clang::InitializePreprocessor` which means > > > `Preprocessor` might need a reference to `OpenCLOptions` which is the > > > right thing to do because it needs to know the features that require the > > > macro, the same as for `LangOpts`. This means we will need to change the > > > initialization time of `OpenCLOptions` member in Sema or potentially even > > > reparent it elsewhere... perhaps to `CompilerInstance` where `LangOpts` > > > and `TargetInfo` seems to be created already? > > > I suggest we move this into clang::InitializePreprocessor which means > > > > I think I'm going to introduce > > `InitializeOpenCLFeatureTestMacros(TargetInfo, LangOptions)` in > > `InitPreprocessor.cpp`. > > > > > This means we will need to change the initialization time of > > > OpenCLOptions member in Sema or potentially even reparent it elsewhere... > > > perhaps to CompilerInstance where LangOpts and TargetInfo seems to be > > > created already > > > > This seems too invasive for me since `CompilerInstance` is a different > > purpose class; and it already holds `Sema` and `TargetInfo`. > > `OpenCLOptions` should mainly be used for parsing, so I would like to > > suggest avoiding using it elsewhere. Furthermore, with the proposed flow > > `OpenCLOptions.h` can be removed later and some simple helpers can be used > > to check the availability of features/extensions. > > > > However, I see your point: we have two identical pieces of code in > > `TargetInfo::getOpenCLFeatureDefines` and `OpenCLOptions::addSupport`. I > > think I'll try to get rid of this duplication by transferring setting of > > core features into `TargetInfo::adjust` which seems pretty right thing to > > do. What do you think? > > This seems too invasive for me since CompilerInstance is a different > > purpose class; and it already holds Sema and TargetInfo. OpenCLOptions > > should mainly be used for parsing, so I would like to suggest avoiding > > using it elsewhere. > > Yes, it is a bigger change I agree. But it is closer to the conventional flow > in my opinion. This is how it works for `LangOpts` and `OpenCLOpts` don't > seem to be different to `LangOpts`, especially now after your refactoring. > Perhaps we should just move `OpenCLOpts` to `LangOpts`? Then `Sema` can > access `OpenCLOpts` through `LangOpts` as well as modify them because they > are immutable. We can then just add the defines along with the rest of > `LangOpts` in `clang::InitializePreprocessor`. > > If you look at `CompilerInvocation` then you will see that while `LangOpts` > are set `TargetInfo` are also taken into account. Then `LangOpts` are > modified using pragmas for example in `Parser::HandlePragmaFPContract()`. > > > Furthermore, with the proposed flow OpenCLOptions.h can be removed later > > and some simple helpers can be used to check the availability of > > features/extensions. > > Ok, this would also be a nice alternative but we will still need to store > information about optional/core versions and etc somewhere? Would it not be > needed for pragmas? > Perhaps we should just move OpenCLOpts to LangOpts? Hmm, this sounds cool. I've never thought of it and this requires some extra investigation. > Ok, this would also be a nice alternative but we will still need to store > information about optional/core versions and etc somewhere? Would it not be > needed for pragmas? Well, since there are not a few extensions which participate in parsing (require pragma) we can keep a separate structure for them. Originally my idea was to simplify `OpenCLOptions` for parsing purposes, so we might not to fully eliminate it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92277/new/ https://reviews.llvm.org/D92277 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
