[PATCH] D91531: [RFC][OpenCL] Provide mechanisms for defining extension macros

Tue Nov 24 07:27:12 PST 2020

Anastasia added a comment.

In D91531#2412532 <https://reviews.llvm.org/D91531#2412532>, @azabaznov wrote:

>> So if I understand it well you are suggesting to perform a check for every parsed macro definition that would identify whether the macro has a name matching what is provided in -cl-ext=? Then if the name matches it would check whether the macro should be defined or not?
>
> No, sorry for confusing. There is no need to check every parsed macro. For example an internal pragma can be used for these purposes (`#pragma OPENCL EXTENSION all : undef` in example below). So imagine if we can differentiate header-only extensions and features and all of them are defined in header. After the list of definitions the special pragma is used and is a processed in a way that it inserts undef for each macro definition which relates to header-only extension/feature and was turned off with the option (`-cl-ext=-cl_khr_depth_images`):
>
>   #define cl_khr_depth_images 1
>   #define cl_khr_fp64 1
>   #define cl_khr_mipmap_image  1
>   ...
>   #pragma OPENCL EXTENSION all : undef
>
> However, this might be hard to maintain and I'm not sure yet that this is even legally to do in current `Preprocessor` design, but this is at least more scalable than adding `#if defined(__undef_` for each extension in the end of the header.  Nevertheless, that's what I meant about preserving the current interface.

Regarding the scalability, I think we can hide the fact that `__undef_<ext name> ` are being added using some mechanisms I have suggested earlier. As for interface, I agree that this will be different. But I don't know whether it is a new problem. For C and C++ it is similar - the functionality is being extended via libraries where possible without modifying the compiler. Adding everything in the compiler is just not going to work. It is just that we ended up doing this more than we should have. Do you see a big value in hooking the extensions to `-cl-ext`? then we could think of some slight modification in its current behavior but I am worried that this might introduce a place for nasty to debug issues.

Regarding the idea of a new pragma I think this is slightly violating normal flow. Such pragma only provides partial functionality i.e. it is useless without `-cl-ext`. The flag is a frontend only so there is no way to set it for the regular users. Also this is something that we should hide from the users really. But there are no good mechanisms to do so. We could, of course, think of some ways to refine the pragma and for that, it is probably better to submit a separate RFC with your initial ideas. But I feel this might bring more problems than it would solve. Also in the past, we have reinvented the wheel several times in OpenCL and the community is just not big enough to maintain everything so if we can reuse functionality from C or C++ we should just do it.

> Also, I didn't quite get how the proposing hook will allow users to declare own extensions outside the codebase. Are you expecting them to use existing `-cl-ext` or `-Dcl_khr_depth_images`? In the later case they won't able to use `#pragma OPENCL EXTESNION cl_khr_depth_images : enable` (while the specification does not describe for which extensions pragma is exactly needed or not, but still)

Vendor extensions that genuinely need the fronted modifications can be added to `OpenCLExtensions.def`, as I am not suggesting to deprecate them. However if extensions can be implemented as libraries then the macro can be added using either:

- Internal header
- Command line flag `-D`
- Target hooks `TargetInfo::getOpenCLExtensionDefines`

I would recommend using either 1 or 2. It is easier to dial with OpenCL C code than dialing with C++ code that adds OpenCL C code.

Regarding the pragma - that should be added only if it is needed and I think most of the current extensions don't need it. In the future, we should probably change the definition of extensions in `OpenCLExtensions.def` to allow specifying whether pragma is to be added or not. So I think we should just stop adding the pragma everywhere for no reason.

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

https://reviews.llvm.org/D91531