[PATCH] D16047: [OpenCL] Add Sema checks for OpenCL 2.0
Anastasia Stulova via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 14 10:54:13 PST 2016
Anastasia added inline comments.
================
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">;
----------------
pxli168 wrote:
> Anastasia wrote:
> > 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">;
> >
> Seems good. But that error message was from spec, this will change it.
You can pick the wording you like. I personally find this one better!
================
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">;
----------------
pxli168 wrote:
> Anastasia wrote:
> > 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"
> OK, it will save few lines.
And more importantly won't populate diagnostics.
================
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)
----------------
pxli168 wrote:
> Anastasia wrote:
> > 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?
> Yes, but it seems all old code write in that way. I just follow the style.
Ok, I think an improvement to the old code is always welcome!
================
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())
----------------
pxli168 wrote:
> Anastasia wrote:
> > 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?
> I don't see any thing talk about the block and condition. Spec only tells about block and selection.
Ok, in that case there is nothing left to do here I guess.
================
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()))
----------------
pxli168 wrote:
> Anastasia wrote:
> > Could you remove this comment?
> OK. but the "|" may seems strange. I just want to explain it.
Doesn't seem strange to me.
================
Comment at: lib/Sema/SemaExpr.cpp:10115
@@ +10114,3 @@
+ // Block.
+ if (S.getLangOpts().OpenCL && S.getLangOpts().OpenCLVersion >= 200 &&
+ Result->isBlockPointerType()) {
----------------
pxli168 wrote:
> Anastasia wrote:
> > The code above seems to do similar. Could we combine into one function/diagnostic?
> Yes, they are. But they live in different function test for operand "&" and "*" and maybe it is hard to merge them together. I have uploaded the full diff as you asked and uou can expand the code to see they are in two function check for the two unary operators.
Yes, it's not a problem that they are in different functions. Could this code be encapsulated in a function, let's say
checkBlockType(...)
and be called from multiple places?
================
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
----------------
pxli168 wrote:
> Anastasia wrote:
> > 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."
> But the example said there can be program scope atomic that only in global address space.
>
> > This macro can only be used to initialize atomic objects that are declared in program scope in the global address space
>
> The quote from spec said so.
Yes, so copying spec into code is not the goal. Let's try to make it shorter and still have the same meaning!
================
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}}
+}
----------------
pxli168 wrote:
> Anastasia wrote:
> > Why using a macro here? It seems to complicate the test without adding any functionality.
> Because the the init type maybe different and then have another error.
>
> I will have a test for this semacheck as I said above.
The macro is removed by the preprocessor and it's empty here anyways.
You are not adding a builtin macro ATOMIC_VAR_INIT to Clang.
This test can look a lot cleaner (without doing any macro expansion):
atomic_int guide = 42;
http://reviews.llvm.org/D16047
More information about the cfe-commits
mailing list