[PATCH] D92277: [OpenCL] Refactor of targets OpenCL option settings
Anton Zabaznov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jan 18 04:35:35 PST 2021
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
More information about the cfe-commits
mailing list