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