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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
