[PATCH] D16047: [OpenCL] Add Sema checks for OpenCL 2.0
Anastasia Stulova via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 13 03:58:20 PST 2016
Anastasia added inline comments.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:593
@@ -592,2 +592,3 @@
+def err_no_declarators : Error<"declaration does not declare anything">;
def ext_typedef_without_a_name : ExtWarn<"typedef requires a name">,
InGroup<MissingDeclarations>;
----------------
I just can't understand the intention here. Could you give any code example or reference to spec?
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7666
@@ +7665,3 @@
+ "dereferencing pointer of type %0 is not allowed in OpenCL">;
+def err_opencl_pointer_to_image : Error<
+ "pointer to image is invalid in OpenCL">;
----------------
I have a feeling that line numbers are broken now in this review due to patch reupload.
Please, see the comment on the line 7670.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7670
@@ +7669,3 @@
+ "%0 can only be used as a function parameter">;
+def err_opencl_atomic_init_addressspace : Error<
+ "initialization of atomic variables is restricted to variables in global address space in opencl">;
----------------
Could you do something like:
def err_atomic_init_constant : Error<
"atomic variable can only be %select{assigned|initialised}0 to a compile time constant"
" in the declaration statement in the program scope">;
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7673
@@ +7672,3 @@
+def err_opencl_block_proto_variadic : Error<
+ "invalid block prototype, variadic arguments are not allowed in opencl">;
+def err_opencl_invalid_block_array : Error<
----------------
in OpenCL
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7674
@@ +7673,3 @@
+ "invalid block prototype, variadic arguments are not allowed in opencl">;
+def err_opencl_invalid_block_array : Error<
+ "array of block is invalid in OpenCL">;
----------------
Could we combine err_opencl_invalid_block_array and err_opencl_pointer_to_image saying something like:
"Declaring a %select{pointer|array}0 of type %1 is not allowed in OpenCL"
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7675
@@ +7674,3 @@
+def err_opencl_invalid_block_array : Error<
+ "array of block is invalid in OpenCL">;
+def err_opencl_ternary_with_block : Error<
----------------
array of block type is ...
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7677
@@ +7676,3 @@
+def err_opencl_ternary_with_block : Error<
+ "blocks cannot be used as expressions in ternary expressions in opencl">;
+
----------------
in OpenCL
================
Comment at: lib/Sema/SemaDecl.cpp:5724
@@ +5723,3 @@
+ R->isPipeType()) {
+ Diag(D.getIdentifierLoc(),
+ diag::err_opencl_type_can_only_be_used_as_function_parameter)
----------------
Some of them have to be. For example for C types that we use differently in CL. But for CL only types do we need to check additionally that it's CL? Do we not know it already?
================
Comment at: lib/Sema/SemaExpr.cpp:6251
@@ -6250,1 +6250,3 @@
+/// \brief Return true if the Expr is block type
+static bool checkBlockType(Sema &S, const Expr *E) {
----------------
a block type
================
Comment at: lib/Sema/SemaExpr.cpp:6299
@@ -6286,3 +6298,3 @@
// Now check the two expressions.
if (LHS.get()->getType()->isVectorType() ||
RHS.get()->getType()->isVectorType())
----------------
I am not sure what the question is?
I think using block in a condition should be disallowed. Could you add this to testing as well?
================
Comment at: lib/Sema/SemaExpr.cpp:6316
@@ +6315,3 @@
+ if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion >= 200) {
+ // should output error for both LHS and RHS, use | instead ||
+ if (checkBlockType(*this, LHS.get()) | checkBlockType(*this, RHS.get()))
----------------
Could you remove this comment?
================
Comment at: lib/Sema/SemaExpr.cpp:7550
@@ -7527,3 +7549,3 @@
LHSType, RHSVecType->getElementType(),
- RHSType))
+ RHSType, Loc))
return RHSType;
----------------
I am not clear about the purpose of this change.
================
Comment at: lib/Sema/SemaExpr.cpp:10061
@@ +10060,3 @@
+ // OpenCL v2.0 s6.12.5 - The unary operators (* and &) cannot be used with a
+ // Block
+ if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion >= 200) {
----------------
Block -> block
================
Comment at: lib/Sema/SemaExpr.cpp:10115
@@ +10114,3 @@
+ // Block.
+ if (S.getLangOpts().OpenCL && S.getLangOpts().OpenCLVersion >= 200 &&
+ Result->isBlockPointerType()) {
----------------
The code above seems to do similar. Could we combine into one function/diagnostic?
================
Comment at: lib/Sema/SemaInit.cpp:6139
@@ +6138,3 @@
+ // OpenCL v2.0 s6.13.11.1 - The ATOMIC_VAR_INIT macro expands to a token
+ // sequence suitable for initializing an atomic object of a type that is
+ // initialization-compatible with value. An atomic object with automatic
----------------
I don't think we need to copy the spec text here. I would like to see some explanation of the code actually.
Could you write something like: "Non-program scope atomic variables can't be initialized."
================
Comment at: lib/Sema/SemaInit.cpp:6147
@@ +6146,3 @@
+ // This macro can only be used to initialize atomic objects that are declared
+ // in program scope in the global address space.
+ // Examples:
----------------
Seems pointless though!
================
Comment at: lib/Sema/SemaInit.cpp:6155
@@ +6154,3 @@
+ TyQualifiers.getAddressSpace() == LangAS::opencl_global;
+ if (!HasGlobalAS && Entity.getKind() == InitializedEntity::EK_Variable &&
+ Args.size() > 0) {
----------------
Should you check if it's a program scope variable? Or alternatively we could disallow non-pointer variables with global AS, which should have already been done already.
================
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"}}
----------------
I see. Would it reduce test coverage? Was it potentially testing the mangling of image types?
I think it's best to change the code instead of completely removing it. May be just remove * so it will be parsed. We seem not to have any test for mangling the images.
================
Comment at: test/Parser/opencl-atomics-cl20.cl:71
@@ +70,3 @@
+void atomic_init_test() {
+ atomic_int guide = ATOMIC_VAR_INIT(42); // expected-error {{initialization of atomic variables is restricted to variables in global address space in opencl}}
+}
----------------
Why using a macro here? It seems to complicate the test without adding any functionality.
================
Comment at: test/SemaOpenCL/invalid-decl.cl:6
@@ +5,2 @@
+ myfun(); // expected-error {{implicit declaration of function 'myfun' is invalid in OpenCL}}
+}
----------------
Yes, I think having it in parser is wrong. Not sure why it's there. You can move it if you wish. Otherwise, I will do afterwards.
http://reviews.llvm.org/D16047
More information about the cfe-commits
mailing list