azabaznov 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;
----------------
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?
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
