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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added inline comments.


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:54
+// Extension associated to a type.
+class TypeExtension<string _Ext> : AbstractExtension<_Ext>;
+
----------------
Anastasia wrote:
> 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?
> Do you think it is work extending the online documentation for that too?

Yes we should, but it is perhaps worth waiting until we have clarified the design wrt feature optionality.


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

https://reviews.llvm.org/D100209