[PATCH] D92277: [OpenCL] Refactor of targets OpenCL option settings
Anastasia Stulova via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 20 03:37:08 PST 2021
Anastasia added inline comments.
================
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;
----------------
azabaznov wrote:
> Anastasia wrote:
> > azabaznov wrote:
> > > azabaznov wrote:
> > > > Anastasia wrote:
> > > > > azabaznov wrote:
> > > > > > 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.
> > > > > 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.
> > > > 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".
> > > >
> > > > then there is no need to extra checking for support of core features
> > >
> > > I mean extra checks in compiler, not in kernel code.
> > > 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?
> >
> >
> > 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?
> 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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92277/new/
https://reviews.llvm.org/D92277
More information about the cfe-commits
mailing list