[PATCH] D60763: Prototype OpenCL BIFs using Tablegen

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a subscriber: joey.
Anastasia added a comment.

Not related to this patch but it might be good to start thinking about testing this functionality properly. In the past we haven't tested the header because it would take a lot of testing time. So I would suggest we keep a light minimal basic testing in regular clang tests as is, but then we either add special build target to tests extra header functionality or add special cmake option (i.e. `LLVM_INCLUDE_OPENCL_BIFS_TESTS`) that would enable such testing with regular `check-clang`. I assume the latter can even be used to avoid generating the extra tables from TableGen minimizing the impact on the clang binary size for the situations OpenCL isn't required in the installation.

Any thoughts?



================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:10
+//===----------------------------------------------------------------------===//
+
+class Type<string Name> {
----------------
I would add a comment describing each section:
- Defining classes
- Instantiating types
- Defining buintin functions
- etc...

Also I am a bit lost in the structure here. I think we should group functionality in sections + document each (as requested above).


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:15
+  int isPointer = 0;
+  string as = "clang::LangAS::Default";
+  int has_rounding = 0;
----------------
can be renamed to `addrSpace`


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:16
+  string as = "clang::LangAS::Default";
+  int has_rounding = 0;
+  int is_integer = 0;
----------------
why is the naming here follows different convention?


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:50
+
+let is_integer = 1 in {
+  def char_t   : Type<"char">;
----------------
may be this can go below or above?


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:65
+  def float_t  : Type<"float">;
+  def double_t : Type<"double">;
+}
----------------
half?


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:82
+// Creates builtins for one argument BIFs, taking and returning the same type.
+multiclass bi_vec<string name, Type ReturnType> {
+  def: Builtin<name, ReturnType, [ReturnType]>;
----------------
again the name is not coherent with the rest.


================
Comment at: clang/include/clang/Basic/OpenCLBuiltins.td:125
+// example 'foo', to show using 'version'
+def: Builtin<"foo_version", int_t, [PointerType<int_t, Global>]>;
+let version = CL20 in {
----------------
I think we need a real BIF here or it has to be removed from the commit for now?


================
Comment at: clang/lib/Sema/SemaLookup.cpp:675
 
+// TODO: Auto-generate this from tablegen
+static QualType OCL2Qual(ASTContext &Context, OpenCLType Ty) {
----------------
Does this mean we have to produce this in `ClangOpenCLBuiltinEmitter.cpp`?


================
Comment at: clang/lib/Sema/SemaLookup.cpp:762
+
+    // TODO: This part is taken from Sema::LazilyCreateBuiltin, maybe refactor it.
+    DeclContext *Parent = Context.getTranslationUnitDecl();
----------------
@joey are you suggesting to use a common helper function?


================
Comment at: clang/lib/Sema/SemaLookup.cpp:777
+      for (unsigned i = 0, e = FT->getNumParams(); i != e; ++i) {
+        ParmVarDecl *parm =
+            ParmVarDecl::Create(Context, New, SourceLocation(), SourceLocation(),
----------------
Variable naming convention is not followed!


================
Comment at: clang/test/SemaOpenCL/builtin-new.cl:15
+kernel void test2(global int* bar) {
+  bar[0] = foo_version(bar);
+}
----------------
I guess we need to find an actual function here. How about `ctz`?

Although, it doesn't have to be CL2.0 actually.


================
Comment at: clang/test/SemaOpenCL/builtin-new.cl:30
+kernel void test5(write_only image2d_t img, int2 coord, float4 colour) {
+  write_imagef(img, coord, colour);
+}
----------------
I think different overloads can be available in different CL version. @svenvh can we already handle this?


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:12
+//
+// This tablegen backend emits Clang OpenCL Builtin checking code.
+//
----------------
Builtin -> builtin function


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:34
+namespace {
+class BuiltinNameEmitter {
+public:
----------------
I think this file deserves more documentation...


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

https://reviews.llvm.org/D60763