This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Refactor of targets OpenCL option settings
ClosedPublic

Authored by azabaznov on Nov 29 2020, 8:34 AM.

Details

Summary

Currently, there is some refactoring needed in existing interface of OpenCL option
settings to support OpenCL C 3.0. The problem is that OpenCL extensions and features
are not only determined by the target platform but also by the OpenCL version.
Also, there are core extensions/features which are supported unconditionally in
specific OpenCL C version. In fact, these rules are not being followed for all targets.
For example, there are some targets (as nvptx and r600) which don't support
OpenCL C 2.0 core features (nvptx.languageOptsOpenCL.cl, r600.languageOptsOpenCL.cl).

After the change there will be explicit differentiation between optional core and core
OpenCL features which allows giving diagnostics if target doesn't support any of
necessary core features for specific OpenCL version.

This patch also eliminates OpenCLOptions instance duplication from TargetOptions.
OpenCLOptions instance should take place in Sema as it's going to be modified
during parsing. Removing this duplication will also allow to generally simplify
OpenCLOptions class for parsing purposes.

Diff Detail

Event Timeline

azabaznov created this revision.Nov 29 2020, 8:34 AM
azabaznov requested review of this revision.Nov 29 2020, 8:34 AM

@Anastasia, there is common functionality with your approach in https://reviews.llvm.org/D91531. I guess these two patches can be unified at some point once we discuss it.

azabaznov edited the summary of this revision. (Show Details)Nov 29 2020, 3:22 PM

Thanks for working on this! It is a very good refactoring that brings OpenCL closer inline with convention design flow. I also suggest removing the dependence on LangOpts. See detailed comments.

clang/include/clang/Basic/OpenCLExtensions.def
42

Let's document the differences in those macros.

clang/include/clang/Basic/OpenCLOptions.h
141–142

My concern is that at this point it reads like only core extensions are being initialized, perhaps we should add another macro which would be common between different extensions even if it might be just an alias to OPENCLCOREEXT_INTERNAL. This will avoid confusion.

148–149

How about creating a type alias for this map with a name reflecting its purpose? It could be used for Features in TargetOptions as well, but this can be done separately.

clang/include/clang/Basic/TargetInfo.h
1438

I feel this comment should get updated?

1511

provided in command line -> provided in command line configuration.

clang/lib/Basic/Targets.cpp
716

Setting all extensions as supported is a corner case so I think moving this into a member function that can be called by virtual targets would be a better fit. Btw since this logic is now moved here do you think we can remove OpenCLOptions::supportAll?

718

We used to do this everywhere, but I find it a bit ugly. In C++ we have many good language features to allow avoiding multiline macros. For example we could define a macro to a lambda that would contain the logic for setting one extension conditionally.

729

If we perform this logic in OpenCLOptions (for example in addSupport) we won't need to pass LangOpts here. Then we won't need to worry about the language being compiled for. This will bring implementation better inline with the conventional design i.e. the target configuration is unaffected by the language or its version. See related comment below.

736

OpenCLOptions::support has similar logic that I think won't be needed there any longer? If so I suggest removing it from that function.

clang/lib/Basic/Targets/AMDGPU.h
307

Since you are on this track already it would be cleaner if the target would just set the features based on the HW support without considering language settings i.e. OpenCL version.

When OpenCLOptions in Sema can be set considering both target features and language version. Those options are then used during parsing. I think it will simplify the interface and implementation too and more importantly avoid cyclic dependency between target options and language options.

clang/lib/Frontend/CompilerInstance.cpp
957 ↗(On Diff #308212)

Ok, again if we don't need to pass LangOpts any more we can keep initialization of these options in TargetInfo::CreateTargetInfo that would allow adhering to the conventional flow which we should be doing!

Anastasia added inline comments.Nov 30 2020, 8:48 AM
clang/include/clang/Basic/TargetOptions.h
65

Your current design brings us closer to Features and FeatureMap. Generally it would be desirable to unify even further, even if it might not be possible to reuse the same fields potentially we could unify the date structures and create some common abstractions for this. Just to be clear, I am not saying we have to do this now or in this patch. However, if we can add some clarifications in the comment here to highlight the differences or even suggest further simplifications in FIXME, that would be great. Perhaps yu have already looked at it and have some insights.

My understanding is that the biggest difference right now that Features are mainly taken from architectural descriptions in LLVM. This could be done for some of OpenCL features but not those that are implemented in software i.e. using libraries. In the future, if we remove such features from Clang as suggested in my RFCs e.g. https://reviews.llvm.org/D91531, the unification between regular Features and OpenCL features might become more pleasable. Another obstacle is SPIR which is a virtual target and has no architecture description. Perhaps we could handle it as a special case it somehow.

69

The name is now inconsistent. We already use extensions and options to refer to these in the code. I don't mind which naming scheme you prefer as soon as we stick to one everywhere.

clang/lib/Basic/Targets/X86.h
362

Calling adjustSupportedOpenCLOpts seems to be required behavior now so how about we make it a default implementation of setSupportedOpenCLOpts in TargetInfo that would call a new member function - say setDefaultSupportedOpenCLOpts, that would be a pure virtual member in TargetInfo but every concrete subclass would need to implement to set the features supported by the specific target?

Or alternatively, subclasses can just call TargetInfo::setSupportedOpenCLOpts in their implementations. It seems the same happens with TargetInfo::initFeatureMap already. However, the first option might be a better match.

FYI I just noticed this review is public, but not linked to cfe-commits. Not sure it is intentional though.

Thanks for working on this! It is a very good refactoring that brings OpenCL closer inline with convention design flow

Thanks for feedback. Though the patch indeed requires refactoring and can't be merged as it is, but the main idea still will be the same.

This patch looks inaccurate mainly because I tried to preserve existing interaction with command line. So we should decide the following:

  • Do we need to allow disabling core features via command line?
  • Do we need to allow target disabling core features (NVPTX does so for OpenCL C 2.0 now)?

I personally think that we shouldn't allow doing so in both cases since it violates the specification of OpenCL C 2.0. In this way the change will be much cleaner because we can use only TargetInfo::getOpenCLFeatureDefines( LangOptions) to define macros for core features and set them in OpenCLOptions::addSupport(StringMap<bool> TargetOpts, LangOptions).

Also, I see benefits TargetInfo::setSupportedOpenCLOpts(LangOptions) as follows: targets will set features for each specific OpenCL C version. This may be helpful if target features differentiate between OpenCL C implementations:

if (CLVer == 200) {
 ...
} else if (CLVer == 300) {
 ...
}

But I'm ok to adjust interface if it's not expected to be a common case in general.

Thanks for working on this! It is a very good refactoring that brings OpenCL closer inline with convention design flow

Thanks for feedback. Though the patch indeed requires refactoring and can't be merged as it is, but the main idea still will be the same.

This patch looks inaccurate mainly because I tried to preserve existing interaction with command line. So we should decide the following:

  • Do we need to allow disabling core features via command line?
  • Do we need to allow target disabling core features (NVPTX does so for OpenCL C 2.0 now)?

I personally think that we shouldn't allow doing so in both cases since it violates the specification of OpenCL C 2.0. In this way the change will be much cleaner because we can use only TargetInfo::getOpenCLFeatureDefines( LangOptions) to define macros for core features and set them in OpenCLOptions::addSupport(StringMap<bool> TargetOpts, LangOptions).

I agree with you. I think we should focus on conformant behavior. Extensions are allowed but should have justification, and should be documented. Considering that OpenCL 3.0 provides finer-grained control of various functionality I think core features should be fixed in OpenCL 2.0 or any other versions.

Also, I see benefits TargetInfo::setSupportedOpenCLOpts(LangOptions) as follows: targets will set features for each specific OpenCL C version. This may be helpful if target features differentiate between OpenCL C implementations:

if (CLVer == 200) {
 ...
} else if (CLVer == 300) {
 ...
}

But I'm ok to adjust interface if it's not expected to be a common case in general.

I don't see why targets would support on not support certain features based on the language version though. I think targets should generally support some functionality or not based on the hardware or runtime setup and then it might be disabled from the user if standards are requested that don't expose it. I would vote for simplicity unless we find a strong case to justify the complexity.

azabaznov updated this revision to Diff 310519.Dec 9 2020, 7:21 AM

Changes in the latest patch:

  1. Core features can't be disabled via command line and target settings, core features are supported based on language version. Thus some tests marked as failed for now. NVPTX and r600 targets should use CL 3.0 since 3d image writes is optional in OpenCL C 3.0.
  1. Explicitly differentiate between core and optional core features.
  1. Refactoring of extensions info setting: extension info is using OpenCL versions mask to indicate in which versions it is core or optional core (instead of single value as it was earlier).
  1. Changed TargetInfo::setSupportedOpenCLOpts interface back to not use LangOptions
azabaznov marked 9 inline comments as done.Dec 9 2020, 7:23 AM

This looks much cleaner than the current flow! Thanks! We should just figure out a better place for defining the macros (see detailed comment in Targets.cpp).

I am adding @cfe-commits since it's an important change to be visible to the community. Other than that I hope @yaxunl and @jvesely would be able to have a look at the amendment in the setup of AMD and NVPTX targets and notify us in case they see any concerns.

clang/include/clang/Basic/OpenCLExtensions.def
46

We should add a spec reference or a note detailing the differences among those different kinds.

clang/include/clang/Basic/OpenCLOptions.h
22

Let's add a comment to document this new enum.

36

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.

126

Btw CLVer is not passed here. Same for the other methods above.

clang/include/clang/Basic/TargetInfo.h
1442

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...

1462

This deserves a comment.

clang/lib/Basic/OpenCLOptions.cpp
23

I think the comment in the header should be enough, otherwise we will struggle to keep them in synch.

88

I feel this is outdated now?

108

Let's add a comment above saying that this adds options supported for the targets.

116

Btw what if the target doesn't support the feature? Do you think we could provide an error?

clang/lib/Basic/Targets.cpp
725

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?

clang/test/Misc/nvptx.languageOptsOpenCL.cl
18

We shouldn't disable the test, but only change it to check the conformant behavior i.e. the expected diagnostics for cl_khr_3d_image_writes should be guarded by the OpenCL version.

yaxunl requested changes to this revision.Dec 10 2020, 1:04 PM
yaxunl added inline comments.
clang/test/Misc/r600.languageOptsOpenCL.cl
26

Pls remove XFAIL

113

what's the expected behavior?

Is cl_khr_3d_image_writes to be always defined for -cl-std=CL2.0?

This revision now requires changes to proceed.Dec 10 2020, 1:04 PM
azabaznov added inline comments.Dec 13 2020, 5:49 AM
clang/include/clang/Basic/OpenCLOptions.h
22

I'm going to change comments all over the place in the next patch. Thanks!

36

Ok, that is reasonable. Will change.

clang/include/clang/Basic/TargetInfo.h
1442

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.

clang/lib/Basic/OpenCLOptions.cpp
116

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

clang/lib/Basic/Targets.cpp
725

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?

clang/test/Misc/r600.languageOptsOpenCL.cl
26

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?

113

Yes, 3d image writes is OpenCL C 2.0 core feature.

yaxunl added inline comments.Dec 13 2020, 6:23 AM
clang/test/Misc/r600.languageOptsOpenCL.cl
26

I think it may be reasonable to remove the run line for CL2.0 and corresponding checks for 2.0 specific core features/extensions from this lit test since r600 does not support CL2.0.

azabaznov updated this revision to Diff 316627.Jan 14 2021, 4:55 AM

Changes in the latest patch:

  1. Removed XFAIL and OpenCL C 2.0 test running for r600 and NVPTX target
  1. Fixed comments.
  1. Core features are being set in TargetInfo::adjust.
azabaznov marked 14 inline comments as done.Jan 14 2021, 5:01 AM
azabaznov added inline comments.
clang/include/clang/Basic/TargetInfo.h
1438

I'll change the naming in changes for OpenCL C 3.0 features.

LGTM about r600 changes. Thanks.

This patch contains very useful improvements to the design along with the behavioral fixes. I see that it opens more opportunities to bring the implementation in line with the conventional flow and therefore simplify the further development of new functionality. More improvements can be easily built on top of this work. I, therefore, suggest we drive towards finalizing this patch asap and I have made a couple of small comments that should hopefully be quick enough to address.

clang/include/clang/Basic/OpenCLExtensions.def
17

I guess you mean that this extension is not against a specific version i.e. applies to all versions?

61

FYI not saying this should be changed now because it is sufficient for the current needs but perhaps we could do something like

OPENCL_GENERIC_EXTENSION(cl_khr_3d_image_writes, 100, {OCL_C_20, OCL_C_31}, {OCL_C_30})

if we start having non-continuous set of versions where a feature is optional or core.

74

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

clang/include/clang/Basic/OpenCLOptions.h
25

I suggest putting this in an anonymous namespace along with encodeOpenCLVersion and OpenCLVersionIsContainedInMask, as they are only to be used locally.

63

A small renaming
OpenCLVersionIsContainedInMask -> isOpenCLVersionContainedInMask

93

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.

145–147

I think this map deserves type alias since it's used in quite many places.

clang/lib/Basic/OpenCLOptions.cpp
97

Maybe this could also be switched to a range based loop along with the one below.

clang/lib/Basic/TargetInfo.cpp
360 ↗(On Diff #316627)

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!

clang/lib/Basic/Targets.cpp
718

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.

720

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.

725

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?

jprice added a subscriber: jprice.Jan 17 2021, 9:28 AM
azabaznov added inline comments.Jan 18 2021, 4:35 AM
clang/include/clang/Basic/OpenCLExtensions.def
17

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.

74

Yes, thanks for noticing. This got here accidentally after rebasing.

clang/include/clang/Basic/OpenCLOptions.h
63

Sure, will change.

93

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?

clang/lib/Basic/TargetInfo.cpp
360 ↗(On Diff #316627)

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.

clang/lib/Basic/Targets.cpp
718

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.

720

this will eliminate the need to create the temporary object OpenCLOptionInfo

This temporary object is needed to check availability of extension (see comment above).

725

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.

azabaznov edited the summary of this revision. (Show Details)Jan 18 2021, 4:38 AM
Anastasia added inline comments.Jan 18 2021, 5:09 AM
clang/include/clang/Basic/OpenCLOptions.h
93

Or you mean to define it as a nested class in OpenCLOptions?

Exactly.

clang/lib/Basic/TargetInfo.cpp
360 ↗(On Diff #316627)

It is correct that the core features should be set unconditionally but not in the TargetInfo. If core features are used for targets that don't support them then it should not succeed silently as it does now i.e. this means we need to know what is supported by the targets.

Setting target features in TargetInfo is correct and should stay. We should not change them here though because the language version doesn't change the target capabilities. It can either expose or hide them from the user but it should not modify targets. This is why TargetInfo is immutable after its creation and this is how it should stay. I think it's better if we remove the code here completely and introduce a diagnostic in the subsequent patches that would just check that the features required in the language version are supported by the target.

If we do this then during the parsing we will only use feature information from OpenCLOptions not the targets, but we will know that the target have support of all the features because the check has been performed earlier.

clang/lib/Basic/Targets.cpp
718

Ok, we can't do this now because OpenCLFeaturesMap is indeed simpler. Hopefully, we can change this in subsequent refactorings if we move OpenCLOptions into LangOpts.

720

Yes I understand. That's why I believe these macros should no longer be defined in the targets and I think it is very doable to avoid this but it requires some more changes (i.e. moving OpenCLOptions into LangOpts). So for now we can leave a FIXME comment to explain the issue.

azabaznov added inline comments.Jan 18 2021, 8:13 AM
clang/lib/Basic/TargetInfo.cpp
360 ↗(On Diff #316627)

I'm not generally against of removing core features set up, but I do have some questions and arguments:

It is correct that the core features should be set unconditionally but not in the TargetInfo

Just to make sure: where do you mean core features should be set unconditionally? 

Setting target features in TargetInfo is correct and should stay. We should not change them here though because the language version doesn't change the target capabilities. It can either expose or hide them from the user but it should not modify targets. This is why TargetInfo is immutable after its creation and this is how it should stay

I agree that TargetInfo should stay immutable during parsing, but for example here, in TargetInfo::adjust, current design already allows to change target capabilities based on language options, so I don't see what is conceptually wrong here.

If core features are used for targets that don't support them then it should not succeed silently as it does now i.e. this means we need to know what is supported by the targets.

My main point in proposed design is that it is closer to specification: if target reports support for OpenCL C 2.0 then there is no need to extra checking for support of core features such as 3d image writes (we could also set for example generic address space and pipes as supported unconditionally later) as it is core in OpenCL C 2.0. Of course this should not be done silently; some diagnostics like fatal error "OpenCL C 2.0 is not supported in this target" or warning "core feature cl_khr_3d_image_writes is not supported in this target".

azabaznov added inline comments.Jan 18 2021, 8:23 AM
clang/lib/Basic/TargetInfo.cpp
360 ↗(On Diff #316627)

then there is no need to extra checking for support of core features

I mean extra checks in compiler, not in kernel code.

Anastasia added inline comments.Jan 19 2021, 3:59 AM
clang/lib/Basic/TargetInfo.cpp
360 ↗(On Diff #316627)

Just to make sure: where do you mean core features should be set unconditionally?

Why not to set them in OpenCLOptions directly? If you don't need any target properties for those why to do this in targets at all? After all OpenCLFeaturesMap will only be used to populate the OpenCLOptions. During the parsing, we will only use OpenCLOptions right?

I agree that TargetInfo should stay immutable during parsing, but for example here, in TargetInfo::adjust, current design already allows to change target capabilities based on language options, so I don't see what is conceptually wrong here.

Well, adjust was added to guarantee type widths for OpenCL. However, it has been introduced as a workaround and it still is. It has that fundamental problem of silently mutating the type without any guarantee of integrity i.e. what if targets doesn't support certain bitwidth? Then the code is just compiled incorrectly. There were various proposals to address this but it needs bigger refactoring - perhaps we should introduce the environment component for OpenCL. While for this use case the refactoring is too big and the workaround is very simple there are other places in clang where we could benefit from having such environment component in targets. So the gain might justify the effort. So in short unless there is a very good reason we should avoid using adjust because we still would like to remove it or at least remove OpenCL logic from it if possible.

My main point in proposed design is that it is closer to specification: if target reports support for OpenCL C 2.0 then there is no need to extra checking for support of core features such as 3d image writes (we could also set for example generic address space and pipes as supported unconditionally later) as it is core in OpenCL C 2.0. Of course this should not be done silently; some diagnostics like fatal error "OpenCL C 2.0 is not supported in this target" or warning "core feature cl_khr_3d_image_writes is not supported in this target".

I disagree spec never says that the standard should be supported on all targets even if they don't have the required functionality neither it regulate implementation aspects of mapping between language versions to targets.

I think the design we should aim for:

  • Targets set supported HW features
  • Frontend verifies the features are present for the language version requested during compilation. If there is a mismatch a diagnostic should occur.
  • Frontend sets language options based on the requested language version and target features that are used during parsing to verify code correctness and create AST.

Do you see any issue with this flow?

azabaznov added inline comments.Jan 20 2021, 1:47 AM
clang/lib/Basic/TargetInfo.cpp
360 ↗(On Diff #316627)

Why not to set them in OpenCLOptions directly? If you don't need any target properties for those why to do this in targets at all? After all OpenCLFeaturesMap will only be used to populate the OpenCLOptions. During the parsing, we will only use OpenCLOptions right?

We also need to add preprocessor define for these extensions so we should look into OpenCLFeaturesMap to add preprocessor defines because target could disable core feature (see my comment below).

I disagree spec never says that the standard should be supported on all targets even if they don't have the required functionality neither it regulate implementation aspects of mapping between language versions to targets.

Well, I think we start going in circles. This was actually my understanding of the difference between optional core and core features - the latter is required to be supported for all implementations from the version when it becomes core (unconditionally). Is my understanding right here? However, there is no clear stating about core features in the spec, this should definitely be added.

Do you see any issue with this flow?

This looks great. In terms of implementation I suggest doing the following: core features are supported unconditionally for certain OpenCL C version, but targets are allowed to disable them:

bool CompilerInstance::ExecuteAction(FrontendAction &Act) {
...
  if (getLangOpts().OpenCL) {
    getTarget().setCoreOpenCLFeatures(getLangOpts());
    getTarget().setSupportedOpenCLOpts();
    getTarget().setCommandLineOpenCLOpts();
  }
  // FIXME: We shouldn't need to do this, the target should be immutable once
  // created. This complexity should be lifted elsewhere.
  getTarget().adjust(getLangOpts());
...
}

...

void AMDGPUTargetInfo::setSupportedOpenCLOpts() {
  // Core feature in CL 2.0, but not supported on amdgpu
  Opts["cl_khr_3d_image_writes"] = false;
  ...
}

Of course proposed flow is relevant if and only if core features concept is correctly interpreted. This will give more flexibility for targets and will also allow to do diagnostics later. Also, this will help to unconditionally support such core features for OpenCL C 2.0 as generic address space and pipes etc. when implementing OpenCL C 3.0. What do you think?

Anastasia added inline comments.Jan 20 2021, 3:37 AM
clang/lib/Basic/TargetInfo.cpp
360 ↗(On Diff #316627)

We also need to add preprocessor define for these extensions so we should look into OpenCLFeaturesMap to add preprocessor defines because target could disable core feature (see my comment below).

Yes, this is because the macro definition is not correctly positioned. We should be able to lift that into clang::InitializePreprocessor with the subsequent changes as discussed earlier. Then we will no longer have this issue.

Well, I think we start going in circles. This was actually my understanding of the difference between optional core and core features - the latter is required to be supported for all implementations from the version when it becomes core (unconditionally). Is my understanding right here? However, there is no clear stating about core features in the spec, this should definitely be added.

I think we have attempted to but it has not been completed, so feel free to provide your feedback https://github.com/KhronosGroup/OpenCL-Docs/issues/500.

However, I feel you are conflating the specification wording that regulates conformant implementation behavior with the implementation detail of supporting various targets that don't necessarily support all standards.

This looks great. In terms of implementation I suggest doing the following: core features are supported unconditionally for certain OpenCL C version, but targets are allowed to disable them:

bool CompilerInstance::ExecuteAction(FrontendAction &Act) {
...

if (getLangOpts().OpenCL) {
  getTarget().setCoreOpenCLFeatures(getLangOpts());
  getTarget().setSupportedOpenCLOpts();
  getTarget().setCommandLineOpenCLOpts();
}
// FIXME: We shouldn't need to do this, the target should be immutable once
// created. This complexity should be lifted elsewhere.
getTarget().adjust(getLangOpts());

...
}

...

void AMDGPUTargetInfo::setSupportedOpenCLOpts() {

// Core feature in CL 2.0, but not supported on amdgpu
Opts["cl_khr_3d_image_writes"] = false;
...

}

If we can make the flow straightforward why would we instead make it twisty? I think after your patch it should be very straightforward to just lift OpenCLOptions in the correct place and fix the macro definitions. I don't see what we gain from setting unsupported features to supported? If the wrong language version is being used for the target it is an incorrect case and the compilation should not be allowed or if it does it can be anything. At least if we don't set the unsupported features some errors are likely to occur if unsupported features are used in the kernel code. Otherwise the parsing just succeeds silently.

Of course, the best way is to provide a dedicated diagnostic and we have done it in Clang for a decade. All the mechanics is in place we just need to fix our flow.

Of course proposed flow is relevant if and only if core features concept is correctly interpreted. This will give more flexibility for targets and will also allow to do diagnostics later. Also, this will help to unconditionally support such core features for OpenCL C 2.0 as generic address space and pipes etc. when implementing OpenCL C 3.0. What do you think?

I am not suggesting by any means to implement non-conformant behavior. I think we should add the features to targets individually for now for OpenCL3.0 because there shouldn't too many? But I think we will be able to do the final fix to correct the flow soon, then we will have more options to avoid duplications (if we need to) and especially to add the correctness checks and the diagnostics.

azabaznov updated this revision to Diff 318165.Jan 21 2021, 5:15 AM
azabaznov marked 7 inline comments as done.

Thanks for feedback. I agree that providing diagnostics for unsupported core features is enough for now (I guess I'll try to come up with something for OpenCL C 3.0 features to set them unconditionally for 2.0, or at least these macros can be defined in header only). So I removed setting of core features (and also addressed all cosmetic concerns).

azabaznov marked 7 inline comments as done.Jan 21 2021, 5:21 AM
Anastasia accepted this revision.Jan 21 2021, 6:53 AM

LGTM! Thanks! Great work! The comment can be addressed before committing.

clang/include/clang/Basic/OpenCLOptions.h
161

I think we no longer need this? If so I suggest removing the function and if needed it can be added later?

clang/test/Misc/nvptx.languageOptsOpenCL.cl
9

We should try to remember restoring some of these RUN lines once we have enough OpenCL 3.0 functionality.

azabaznov updated this revision to Diff 318458.Jan 22 2021, 1:39 AM

Removed getCoreFeatures() as it's not used in this change.

Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2021, 1:39 AM
azabaznov marked an inline comment as done.Jan 22 2021, 1:42 AM
azabaznov added inline comments.
clang/test/Misc/nvptx.languageOptsOpenCL.cl
18

Yeah, sure. This won't a problem in this case as these test contain OpenCL C 1.2 runs. OpenCL C 3.0 is backward compatible with OpenCL C 1.2 so all 1.2 runs should be tested together with 3.0.

azabaznov edited the summary of this revision. (Show Details)Jan 22 2021, 1:51 AM
Anastasia accepted this revision.Jan 22 2021, 2:21 AM

Cool. Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Jan 25 2021, 8:50 AM
This revision was automatically updated to reflect the committed changes.