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

Wed Feb 2 01:53:31 PST 2022

azabaznov 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).
----------------
Anastasia wrote:
> 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.
> 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.

So yeah, I think reusing LanguageID is pretty doable and sounds like a good idea.

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

The whole problem is that features used by TARGET_BUILTIN are defined by targets in llvm, for example avx512vl in X86. So making each target to define, for example, //__opencl_c_device_enqueue// is not possible. The other approach might be to outline the list of common subtarget features shared by all targets, which is also not a good solution (despite the fact that there are already some of those), but OpenCL features would fit into such concept.

We discussed this a lot, but IMO OpenCL features are more about the language, not about the target. For example, target  might not support fp64, but //__opencl_c_fp64// is supported so target can emulate fp64 (not a good example, but I imagine this is possible). With this fact given, I thing LANGBUILTIN fits better here then TARGET_BUILTIN.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118605