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