[PATCH] D96524: [OpenCL] Add support of OpenCL C 3.0 __opencl_c_fp64

Wed Apr 21 07:42:50 PDT 2021

azabaznov added a comment.

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96524/new/

https://reviews.llvm.org/D96524