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

Sven van Haastregt via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 13 07:21:44 PDT 2021


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100209



More information about the cfe-commits mailing list