[cfe-dev] OpenCL vec_step feature implementation

[Anton Lokhmotov]

Hi Guy,

> We'd like to get your feedback, and eventually we'd like to contribute
> this code to the open source development trunk.
It's great to see more companies/people contributing to OpenCL support in
Clang!

> Since vec_step might take types as argument, it cannot be implemented
> as a standard function, it is rather similar to sizeof and alignof
> expressions.  In this implementation, we take advantage of the existing 
> SizeOfAlignOfExpr class and the functions manipulating it, and we add
> vec_step to this expression class.
Agree.  In fact, your implementation is very similar to ours :).

> We might change the name to something like UnaryOperatorWithExprOrType
> - still long and ugly, but generic. I'll be happy to get your ideas about
> the name.
Perhaps UnaryOperatorOnExprOrType?

Couple of minor comments inline.

Cheers,
Anton.

===================================================================
--- include/clang/AST/Expr.h	(revision 125563)
+++ include/clang/AST/Expr.h	(working copy)
... 
 /// SizeOfAlignOfExpr - [C99 6.5.3.4] - This is for sizeof/alignof, both of
 /// types and expressions.
Also [OpenCL 6.11.12]... once the name is changed ;).

===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td	(revision 125563)
+++ include/clang/Basic/DiagnosticSemaKinds.td	(working copy)
...
+def err_vecstep_not_in_opencl : Error<
+  "usage of vec_step operator is allowed in OpenCL only">;

In fact, vec_step is also used in AltiVec (see PIM 2.5.3, where it is
properly called an operator).

===================================================================
--- include/clang/Basic/TokenKinds.def	(revision 125563)
+++ include/clang/Basic/TokenKinds.def	(working copy)
...
-#undef TOK
+#undef TOK
\ No newline at end of file

???

===================================================================
--- lib/AST/StmtDumper.cpp	(revision 125563)
+++ lib/AST/StmtDumper.cpp	(working copy)
@@ -448,7 +448,19 @@
 }
 void StmtDumper::VisitSizeOfAlignOfExpr(SizeOfAlignOfExpr *Node) {
   DumpExpr(Node);
-  OS << " " << (Node->isSizeOf() ? "sizeof" : "alignof") << " ";
+  switch(Node->getKind()) {
+	  case SizeOfAlignOfExpr::SizeOf:
Extra space before case here. (Ditto in lib/AST/StmtPrinter.cpp.)

===================================================================
--- test/CodeGenOpenCL/opencl_vecstep.cl	(revision 0)
+++ test/CodeGenOpenCL/opencl_vecstep.cl	(revision 0)
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 %s -emit-llvm -O0 -o - | FileCheck %s
+
+typedef int int2 __attribute__((ext_vector_type(2)));
+typedef int int3 __attribute__((ext_vector_type(3)));
+typedef int int4 __attribute__((ext_vector_type(4)));
+typedef int int8 __attribute__((ext_vector_type(8)));
+typedef int int16 __attribute__((ext_vector_type(16)));
+
+void foo(int3 arg1, int8 arg2) {
+  int4 auto1;
+  int16 *auto2;
+  int auto3;
Please also check for applying vec_step() to an int2 variable or argument.

+  int res4 = vec_step(auto2);
+// CHECK: store i32 1, i32* %res4
...
+  int res11 = vec_step(&auto1);
+// CHECK: store i32 1, i32* %res11

I'd argue that passing a pointer type to the vec_step() operator should
generate an error, because according to the OpenCL specification vec_step()
can only take built-in scalar and vector types,  and pointer types are
derived.  I suspect that applying vec_step() to types void, bool, size_t,
ptrdiff_t, intptr_t and uintptr_t should be rejected as well.