Anastasia added a comment. In D96524#2705192 <https://reviews.llvm.org/D96524#2705192>, @azabaznov wrote:
> In D96524#2692376 <https://reviews.llvm.org/D96524#2692376>, @Anastasia wrote: > >> In D96524#2691428 <https://reviews.llvm.org/D96524#2691428>, @azabaznov >> wrote: >> >>> My main idea was to provide an interface which will not make users to >>> specify `-cl-ext=+__opencl_c_fp64,+cl_khr_fp64`/ >>> `-cl-ext=-__opencl_c_fp64,-cl_khr_fp64` if they need to enable/disable >>> functionality in OpenCL C 3.0 because I believe that is not a right thing >>> to do: why anyone should care about those extensions if there are features >>> already? Also, this may lead to confusions since, for example, >>> `__opencl_c_subgroups` and `cl_khr_subgroups `are not the same (subgroup >>> independent forward progress is required in extension while it is optional >>> in OpenCL C 3.0, thus implementation may support the extension but not the >>> feature). >>> >>> To be clear: I'm OK with providing a validation of correct option settings >>> (`__opencl_c_fp64/cl_khr_fp64` and >>> `__opencl_c_3d_image_writes/cl_khr_3d_image_writes` should both be set to >>> the same value). Also, it makes sense to unify a check within header and >>> clang to the only macro, I'm OK with that too. But I would prefer to keep >>> the option interface without redundant mentioning of extension. >> >> Ok, let's implement both - we can add an early check of consistency of >> options and therefore will only need to use one for checking during the >> parsing and let's simplify the interface of `-cl-ext`. Since it is a >> frontend option and new functionality doesn't cause backward compatibility >> issues i.e. only affects OpenCL 3.0 onwards, so I see no risk. Could you >> please update the help documentation of the option explaining the new >> behavior? Then let's concentrate the whole logic for setting the options and >> keeping the consistency between the equivalent ones in >> `TargetInfo::setCommandLineOpenCLOpts` directly? This will provide cleaner >> flow due to encapsulation and will make it clear where the use case comes >> from. Does it make sense? > > Sure, agree. FYI I'm looking to a proper way of validating a target already. > There already exists `TargetInfo::validateTarget`. I wanted to reuse that > (just don't want to introduce yet another interface for OpenCL in Target), > but I'm again bumping into some problems because of our //special// OpenCL > options settings: a target should be immutable once created; and language > options are not used to create a target. I'm just wondering if there is some > more refactoring is needed... I see, if you need any help feel free to provide more details and we can discuss it further. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96524/new/ https://reviews.llvm.org/D96524 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
