azabaznov added inline comments.
================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:22
-/// OpenCL supported extensions and optional core features
-class OpenCLOptions {
- struct Info {
- bool Supported; // Is this option supported
- bool Enabled; // Is this option enabled
- unsigned Avail; // Option starts to be available in this OpenCL version
- unsigned Core; // Option becomes (optional) core feature in this OpenCL
- // version
- Info(bool S = false, bool E = false, unsigned A = 100, unsigned C = ~0U)
- :Supported(S), Enabled(E), Avail(A), Core(C){}
- };
- llvm::StringMap<Info> OptMap;
-public:
- /// Check if \c Ext is a recognized OpenCL extension.
- ///
- /// \param Ext - Extension to look up.
- /// \returns \c true if \c Ext is known, \c false otherwise.
- bool isKnown(llvm::StringRef Ext) const {
- return OptMap.find(Ext) != OptMap.end();
- }
+enum OpenCLVersionID : unsigned int {
+ OCL_C_10 = 0x1,
----------------
Anastasia wrote:
> Let's add a comment to document this new enum.
I'm going to change comments all over the place in the next patch. Thanks!
================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:36
+// OpenCL C version mask
+class OpenCLVersionEncoderHelper {
+private:
----------------
Anastasia wrote:
> Looking at the uses, it might be better to just have a helper function that
> takes `OpenCLVersion` and `LangOpts` instead of introducing a class that is
> only used as temporary.
Ok, that is reasonable. Will change.
================
Comment at: clang/include/clang/Basic/TargetInfo.h:1442
+ virtual void supportAllOpenCLOpts(bool V = true) {
+#define OPENCLEXTNAME(Ext) getTargetOpts().OpenCLFeaturesMap[#Ext] = V;
+#include "clang/Basic/OpenCLExtensions.def"
----------------
Anastasia wrote:
> Btw here you could just iterate over the elements in the map? This could be
> faster than multiple independent look ups into the map. But mainly I think it
> makes the flow a bit cleaneras it indicated that you only set elements and
> not add them...
> Btw here you could just iterate over the elements in the map
Map is empty at this point. `::supportAllOpenCLOpts` adds all elements which
are declared in `OpenCLExtensions.def` into the map.
================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:116
+ if (F.getValue().IsCoreIn(Opts))
+ support(F.getKey());
+}
----------------
Anastasia wrote:
> Btw what if the target doesn't support the feature? Do you think we could
> provide an error?
I think that if a feature is //core// than a target //must// support it, do I
miss something?
However, there is a place for such diagnostics for non-core features already:
something similar to `TargetInfo::handleTargetFeatures(std::vector<std::string>
&Features, DiagnosticsEngine &Diags)` could be used
================
Comment at: clang/lib/Basic/Targets.cpp:725
+ if ((It != OpenCLFeaturesMap.end() && It->getValue()) ||
+ OpenCLOptionInfo(AvailVer, CoreVersions, OptionalVersions)
+ .IsCoreIn(Opts))
----------------
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?
================
Comment at: clang/test/Misc/r600.languageOptsOpenCL.cl:26
// RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple r600-unknown-unknown
-Wpedantic-core-features -DTEST_CORE_FEATURES -target-cpu turks
+// XFAIL: *
----------------
yaxunl wrote:
> Pls remove XFAIL
Sure. I'll guard this #ifndef check with OpenCL version in existing tests; but
however, I think adding new tests with XFAIL for r600 and NVPTX (where #ifndef
check fails) seems reasonable to me. What do you think?
================
Comment at: clang/test/Misc/r600.languageOptsOpenCL.cl:113
+// FIXME: this test fails because 3d image writes is core
+// feature in CL 2.0. Target should be using CL 3.0.
#ifdef cl_khr_3d_image_writes
----------------
yaxunl wrote:
> what's the expected behavior?
>
> Is cl_khr_3d_image_writes to be always defined for -cl-std=CL2.0?
Yes, 3d image writes is OpenCL C 2.0 core feature.
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
