[PATCH] D16047: [OpenCL] Add Sema checks for OpenCL 2.0
Anastasia Stulova via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 12 02:58:30 PST 2016
Anastasia added a comment.
Also generally it's much nicer to have small logically isolated changes committed. I can see how you could partition this change into into pipe, blocks and images.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:593
@@ -592,2 +592,3 @@
InGroup<MissingDeclarations>;
+def err_no_declarators : Error<"declaration does not declare anything">;
def ext_typedef_without_a_name : ExtWarn<"typedef requires a name">,
----------------
Can you explain why you are adding this and not relying on standard C behavior? Any reference to spec or complete example would be helpful!
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7666
@@ +7665,3 @@
+def err_opencl_atomic_init_addressspace : Error<
+ "initialization of atomic variables is restricted to variables in global address space in opencl">;
+def err_opencl_block_proto_variadic : Error<
----------------
I think it's best to merge this with err_atomic_init_constant diagnostic. You can have {assigned|initialize} in the text and pass which value to select in the code handling the error.
I would also rename it directly to: err_opencl_atomic_init_constant
================
Comment at: lib/Sema/SemaDecl.cpp:5724
@@ +5723,3 @@
+ // Pipes can only be passed as arguments to a function.
+ if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion >= 200 &&
+ R->isPipeType()) {
----------------
is CL check really needed since we are accepting pipes only in CL2.0?
================
Comment at: lib/Sema/SemaDecl.cpp:5735
@@ +5734,3 @@
+
+ // OpenCL v1.2 s6.5 p5
+ // There is no generic address space name for program scope variables.
----------------
Dead code here?
================
Comment at: lib/Sema/SemaDecl.cpp:6745
@@ +6744,3 @@
+ const BlockPointerType *BlkTy = T->getAs<BlockPointerType>();
+ assert(BlkTy && "Not a block pointer.");
+
----------------
this seems to be redundant considering the check above.
================
Comment at: lib/Sema/SemaDecl.cpp:6760
@@ +6759,3 @@
+#if 0
+ // OpenCL v2.0 s6.9.b
+ // An image type can only be used as a type of a function argument.
----------------
Dead code!
================
Comment at: lib/Sema/SemaDecl.cpp:7302
@@ -7209,2 +7302,2 @@
QualType PointeeType = PT->getPointeeType();
if (PointeeType->isPointerType())
----------------
I feel like exporting the full diff might be a good idea here. A lot of small framents hard to understand.
"To get a full diff, use one of the following commands (or just use Arcanist to upload your patch):
git diff -U999999 other-branch
svn diff --diff-cmd=diff -x -U999999"
================
Comment at: lib/Sema/SemaDecl.cpp:7308
@@ +7307,3 @@
+ unsigned addrSpace = PointeeType.getAddressSpace();
+ return (addrSpace != LangAS::opencl_global &&
+ addrSpace != LangAS::opencl_constant &&
----------------
Why this change?
================
Comment at: lib/Sema/SemaDecl.cpp:11466
@@ +11465,3 @@
+ else if (getLangOpts().OpenCL)
+ // OpenCL spir-function need to be called with prototype, so we don't allow
+ // implicit function declarations in OpenCL
----------------
Can you remove "spir-" from here?
================
Comment at: lib/Sema/SemaExpr.cpp:6299
@@ +6298,3 @@
+ if (checkBlockType(*this, LHS.get()) | checkBlockType(*this, RHS.get()))
+ return QualType();
+ }
----------------
Can we produce the diagnostic here and let checkBlockType only return true or false?
================
Comment at: lib/Sema/SemaExpr.cpp:10094
@@ -10060,1 +10093,3 @@
Result = PT->getPointeeType();
+ // OpenCL v2.0 s6.12.5 --The unary operators (* and &) cannot be used with a
+ // Block.
----------------
Remove one -, add space after
================
Comment at: lib/Sema/SemaInit.cpp:6138
@@ -6137,1 +6137,3 @@
+ // OpenCL v2.0 s6.13.1.1 -- This macro can only be used to initialize atomic
+ // objects that are declared in program scope in the global address space.
----------------
I guess you mean s6.13.11.1?
================
Comment at: lib/Sema/SemaInit.cpp:6139
@@ +6138,3 @@
+ // OpenCL v2.0 s6.13.1.1 -- This macro can only be used to initialize atomic
+ // objects that are declared in program scope in the global address space.
+ if (S.getLangOpts().OpenCL && S.getLangOpts().OpenCLVersion >= 200 &&
----------------
Not clear about the macro. Could you be more generic here i.e. write about initialization is generally disallowed.
================
Comment at: lib/Sema/SemaInit.cpp:6143
@@ +6142,3 @@
+ Qualifiers TyQualifiers = Entity.getType().getQualifiers();
+ bool HasGlobalAS = TyQualifiers.hasAddressSpace() &&
+ TyQualifiers.getAddressSpace() == LangAS::opencl_global;
----------------
It would be sufficient to check: TyQualifiers.getAddressSpace() == LangAS::opencl_global
================
Comment at: lib/Sema/SemaInit.cpp:6145
@@ +6144,3 @@
+ TyQualifiers.getAddressSpace() == LangAS::opencl_global;
+ if (!HasGlobalAS && Entity.getKind() == InitializedEntity::EK_Variable &&
+ Args.size() > 0) {
----------------
Not sure about the InitializedEntity::EK_Variable check. What happens if it's removed?
================
Comment at: lib/Sema/SemaInit.cpp:6145
@@ +6144,3 @@
+ TyQualifiers.getAddressSpace() == LangAS::opencl_global;
+ if (!HasGlobalAS && Entity.getKind() == InitializedEntity::EK_Variable &&
+ Args.size() > 0) {
----------------
Anastasia wrote:
> Not sure about the InitializedEntity::EK_Variable check. What happens if it's removed?
Do we need to check if it's a top level declaration and not enclosed in any function?
Also we seem to allow this currently:
kernel void foo(){ global int x;}
I am not sure it's correct.
================
Comment at: lib/Sema/SemaInit.cpp:6147
@@ +6146,3 @@
+ Args.size() > 0) {
+ const Expr *Init = Args[0];
+ S.Diag(Init->getLocStart(), diag::err_opencl_atomic_init_addressspace)
----------------
this variable decl can be avoided
================
Comment at: test/CodeGenOpenCL/opencl_types.cl:39
@@ -38,3 @@
-
-void __attribute__((overloadable)) bad1(image1d_t *b, image2d_t *c, image2d_t *d) {}
-// CHECK-LABEL: @{{_Z4bad1P11ocl_image1dP11ocl_image2dS2_|"\\01\?bad1@@\$\$J0YAXPE?APAUocl_image1d@@PE?APAUocl_image2d@@1 at Z"}}
----------------
Why removing this here?
================
Comment at: test/SemaOpenCL/invalid-decl.cl:10
@@ +9,3 @@
+
+void test2(image1d_t *i){} // expected-error {{pointer to image is invalid in OpenCL}}
+
----------------
can we merge with some image tests?
================
Comment at: test/SemaOpenCL/invalid-decl.cl:13
@@ +12,3 @@
+void test3() {
+ pipe int p; // expected-error {{pipe can only be used as a function parameter}}
+ image1d_t i; // expected-error {{image can only be used as a function parameter}}
----------------
this can be moved to pipe test?
================
Comment at: test/SemaOpenCL/invalid-decl.cl:18
@@ +17,3 @@
+void kernel test4() {
+ atomic_int guide = ATOMIC_VAR_INIT(42); // expected-error {{initialization of atomic variables is restricted to variables in global address space in opencl}}
+}
----------------
Can we just do normal initialization without the macro?
Also please move this into test/Parser/opencl-atomics-cl20.cl to have them all in one place!
http://reviews.llvm.org/D16047
More information about the cfe-commits
mailing list