[PATCH] D100209: [OpenCL] Do not add builtins with unavailable types

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:54
+// Extension associated to a type.
+class TypeExtension<string _Ext> : AbstractExtension<_Ext>;
+
----------------
svenvh wrote:
> Anastasia wrote:
> > I am trying to understand why would we need a special abstraction for the type? Would it not be easier if we just guard the BIFs by the extensions that allow the use of the type? 
> > 
> > We would need to separate the definitions of course but this can be more helpful in order to understand what overloads are available conditionally?
> Yes, it is possible to achieve the same with the current `FunctionExtension` class.
> 
> However, that would require a lot of duplication in the Builtin descriptions, as for example many math builtins have overloads for `half` and `double` that will have to be conditionalized.  The purpose of `TypeExtension` is precisely to avoid separating and duplicating the definitions.  For example, `acos` is currently more or less defined as:
> 
> ```
> def FGenTypeN : GenericType<"FGenTypeN", TypeList<[Float, Double, Half]>, VecAndScalar>;
> def : Builtin<"acos", [FGenTypeN, FGenTypeN], Attr.Const>;
> ```
> with `double` and `half` conditionalization conveniently handled in a single place through a `TypeExtension`.
> 
> If we would only use `FunctionExtension`s, the definition would become more like the following:
> ```
> def FGenTypeN : GenericType<"FGenTypeN", TypeList<[Float]>, VecAndScalar>;
> def : Builtin<"acos", [FGenTypeN, FGenTypeN], Attr.Const>;
> 
> let Extension = Fp64 in {
>   def DGenTypeN : GenericType<"DGenTypeN", TypeList<[Double]>, VecAndScalar>;
>   def : Builtin<"acos", [DGenTypeN, DGenTypeN], Attr.Const>;
> }
> 
> let Extension = Fp16 in {
>   def HGenTypeN : GenericType<"HGenTypeN", TypeList<[Half]>, VecAndScalar>;
>   def : Builtin<"acos", [HGenTypeN, HGenTypeN], Attr.Const>;
> }
> ```
> 
> I personally don't think there is value in adding these explicit guards for every conditional builtin, as the duplication makes the definitions harder to maintain.  In addition, I expect it would also increase the size of the generated tables, as the `GenericType`s have to be split up (though I have not measured this).
Ok I see. Yeah it looks quite a lot of duplication indeed for some types. However, this patch provides extra flexibility and doesn't enforce to use the new approach evrywhere so I think it is very reasonable.

Could you just provide a bit more documentation and especially the difference to the other approach here?

Do you think it is work extending the online documentation for that too?


================
Comment at: clang/lib/Sema/SemaLookup.cpp:762
 
+  if (RetTypes.size() == 0)
+    return;
----------------
This deserved the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100209