[OpenCL] Refactor of targets OpenCL option settings

[azabaznov]

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 feature are not only determined by the target platform but also by the OpenCL version. Also, there are core extensions/features which are guaranteed to be supported by the target with 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):

...
// Core feature in CL 2.0, but not supported on nvptx
#ifdef cl_khr_3d_image_writes
#error "Incorrect cl_khr_3d_image_writes define"
#endif
...

This is erroneous since cl_khr_3d_image_writes is an OpenCL C 2.0 core feature. IMO these targets can't report OpenCL C 2.0 support, and they should use OpenCL C 3.0 since 3d image writes is optional feature in OpenCL C 3.0.

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.

[azabaznov]

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

[Anastasia]

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

46

Let's document the differences in those macros.

clang/include/clang/Basic/OpenCLOptions.h

102–103

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.

107–108

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?

1512

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

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

726

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?

728

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.

739

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.

746

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

309

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

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]

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

[azabaznov]

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.

[Anastasia]

In D92277#2425416, @azabaznov wrote:

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]

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.

2. Explicitly differentiate between core and optional core features.

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

4. Changed TargetInfo::setSupportedOpenCLOpts interface back to not use LangOptions

[Anastasia]

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

50

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

clang/include/clang/Basic/OpenCLOptions.h

22–34

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

39

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.

119

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

clang/include/clang/Basic/TargetInfo.h

1448

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

24

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

89

I feel this is outdated now?

109

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

117

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

clang/lib/Basic/Targets.cpp

735

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

14

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]

clang/test/Misc/r600.languageOptsOpenCL.cl

20

Pls remove XFAIL

116

what's the expected behavior?

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

[azabaznov]

clang/include/clang/Basic/OpenCLOptions.h

22–34

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

39

Ok, that is reasonable. Will change.

clang/include/clang/Basic/TargetInfo.h

1448

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

117

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

735

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

20

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?

116

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

[yaxunl]

clang/test/Misc/r600.languageOptsOpenCL.cl

20

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]

Changes in the latest patch:

1. Removed XFAIL and OpenCL C 2.0 test running for r600 and NVPTX target

2. Fixed comments.

3. Core features are being set in TargetInfo::adjust.

[azabaznov]

clang/include/clang/Basic/TargetInfo.h

1438

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

[yaxunl]

LGTM about r600 changes. Thanks.

[Anastasia]

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?

65

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.

79

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.

55

A small renaming
OpenCLVersionIsContainedInMask -> isOpenCLVersionContainedInMask

62

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.

106

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

clang/lib/Basic/OpenCLOptions.cpp

96

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

clang/lib/Basic/TargetInfo.cpp

360

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

728

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.

730

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.

735

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?

[azabaznov]

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.

79

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

clang/include/clang/Basic/OpenCLOptions.h

55

Sure, will change.

62

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

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

728

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.

730

this will eliminate the need to create the temporary object OpenCLOptionInfo

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

735

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.

[Anastasia]

clang/include/clang/Basic/OpenCLOptions.h

62

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

Exactly.

clang/lib/Basic/TargetInfo.cpp

360

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

728

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.

730

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]

clang/lib/Basic/TargetInfo.cpp

360

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]

clang/lib/Basic/TargetInfo.cpp

360

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

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

[Anastasia]

clang/lib/Basic/TargetInfo.cpp

360

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?