[PATCH] D118605: [OpenCL] Add support of language builtins for OpenCL C 3.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/Basic/Builtins.def:88
 //       '__builtin_' prefix. It will be implemented in compiler-rt or libgcc.
+//  G -> this function uses generic address space (OpenCL).
+//  P -> this function uses pipes (OpenCL).
----------------
azabaznov wrote:
> Anastasia wrote:
> > Anastasia wrote:
> > > It might be better to avoid adding such limited language-specific functionality into generic representation of Builtins. Do you think could we could just introduce specific language modes, say:
> > > 
> > > `OCLC_PIPES`
> > > `OCLC_DSE`
> > > `OCLC_GAS`
> > > 
> > > and then check against those in `builtinIsSupported`?
> > Btw another approach could be to do something similar to `TARGET_BUILTIN` i.e. list features in the last parameter as strings. We could add a separate macro for such builtins and just reuse target Builtins flow. This might be a bit more scalable in case we would need to add more of such builtins later on?
> > 
> > It might be better to avoid adding such limited language-specific functionality into generic representation of Builtins.
> 
> Nice idea! Though I think LanguageID is not designed to be used this way, it's used only to check against specific language version. So it seems pretty invasive. Also, function attributes seem more natural to me to specify the type of the function. I don't know for sure which way is better...
> 
> > Btw another approach could be to do something similar to TARGET_BUILTIN i.e. list features in the last parameter as strings.
> 
> I'd prefer to not use TARGET_BUILTIN as it operates on target feature, but OpenCL feature is some other concept in clang...
Buitlins handling is pretty vital for clang so if we extend common functionality for just a few built-in functions it might not justify the overhead in terms of complexity, parsing time or space... so we would need to dive in those aspects more before finalizing the design... if we can avoid it we should try... and I feel in this case there might be some good ways to avoid it.

> Nice idea! Though I think LanguageID is not designed to be used this way, it's used only to check against specific language version. So it seems pretty invasive. Also, function attributes seem more natural to me to specify the type of the function. I don't know for sure which way is better...

I think this `LanguageID` is only used for the purposes of Builtins, so there should be no issue in evolving it differently. With the documentation and  adequate naming we can resolve the confusions in any.

The whole idea of language options in clang is that it is not dependent on the target. But we have violated this design already. The whole concept of OpenCL 3.0 language features that are target-specific is misaligned with the original design in clang.

> 
>     Btw another approach could be to do something similar to TARGET_BUILTIN i.e. list features in the last parameter as strings.
> 
> I'd prefer to not use TARGET_BUILTIN as it operates on target feature, but OpenCL feature is some other concept in clang...

But we also have target features mirroring these, right? So I see no reason not to reuse what we already have... instead of adding another way to do the same or very similar thing...

We could also consider extending the functionality slightly to use language features instead however I can't see the immediate benefit at this point... other than it might be useful in the future... but we can't know for sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118605