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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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

> Yes, in general this approach looks good to me conceptually. I have two suggestions:
>
> 1. As we discussed, the term //core functionality// should be revisited here. There's no clear meaning about that in the spec and I think interpreting it as //supported by default// is a little dangerous. So //core// (AFAIK) means that it was just promoted to a core specification thus is still remains optional by targets.

Btw I think there is a core and an optional core extension too? I believe that the definition that you are providing applies to the optional core extension but not core extension. However I have created this issue as I think the spec should be explicit about all of these: https://github.com/KhronosGroup/OpenCL-Docs/issues/500.

> 1. Sort of a implementation suggestion. I understand that double-scored identifiers are reserved for any use, but still, can defining such macro as `__undef_cl_khr_depth_images ` be avoided? We could use `Preproceccor` class for the things that you are proposing to do. I was trying to do something similar when implementing features and I tried something like (`Preprocessor::appendDefMacroDirective` already exists):
>
>
>
>   UndefMacroDirective *Preprocessor::appendUndefMacroDirective(IdentifierInfo *II,
>                                  SourceLocation Loc) {
>      if (!II->hasMacroDefinition())
>        return nullptr;
>      UndefMacroDirective *UD = AllocateUndefMacroDirective(Loc);
>      appendMacroDirective(II, UD);
>      return UD;
>   }
>
> I tried to handle some special pragma in this way and it worked. So maybe this can be reused without binding to any specific `SourceLocation`? But maybe there is an other strong concern why `Preprocessor::appendUndefMacroDirective` still doesn't exist...

As far as I can see `Preprocessor::appendDefMacroDirective` is mainly used for the extension https://gcc.gnu.org/onlinedocs/gcc/Push_002fPop-Macro-Pragmas.html. It seems interesting but I am not sure yet if it can help.  So what do you plan to do with `Preprocessor::appendUndefMacroDirective`? Perhaps if you give me an example it would help to understand.

I agree that `__undef` macro is a bit messy. We could of course make it a bit prettier. For example,

(1.) we could add a helper macro

  #define DEFINE_EXTENSION_MACRO(NAME) \
  #if !defined(NAME) && !defined(__undef_ ## NAME)\
  #define NAME \
  #endif

Then we could make a definition of `cl_khr_depth_images` simpler

  #if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0) || \
      (__OPENCL_C_VERSION__ >= CL_VERSION_1_2  && defined(__SPIR__) )
  DEFINE_EXTENSION_MACRO(cl_khr_depth_images)
  #endif

or (2.) just do something like the following at the end of all extension/feature macro setting code.

  #if defined(__undef_cl_khr_depth_images)
  #undef cl_khr_depth_images
  #endif


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

https://reviews.llvm.org/D91531