Add Sema checks for opencl 2.0 new features: Block, pipe, atomic etc. Also fix some old Sema check like pointer to image.
This patch is based on bader's patch in SPIRV-1.0 branch.
Details
- Reviewers
Anastasia pekka.jaaskelainen
Diff Detail
Event Timeline
lib/Sema/SemaExpr.cpp | ||
---|---|---|
6298 |
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.
include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
593 | 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! | |
7666 | 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 | |
lib/Sema/SemaDecl.cpp | ||
5724 | is CL check really needed since we are accepting pipes only in CL2.0? | |
5735 | Dead code here? | |
6745 | this seems to be redundant considering the check above. | |
6760 | Dead code! | |
7302 | 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" | |
7308 | Why this change? | |
11466 | Can you remove "spir-" from here? | |
lib/Sema/SemaExpr.cpp | ||
6299 | Can we produce the diagnostic here and let checkBlockType only return true or false? | |
10094 | Remove one -, add space after | |
lib/Sema/SemaInit.cpp | ||
6138 | I guess you mean s6.13.11.1? | |
6139 | Not clear about the macro. Could you be more generic here i.e. write about initialization is generally disallowed. | |
6143 | It would be sufficient to check: TyQualifiers.getAddressSpace() == LangAS::opencl_global | |
6145 | Not sure about the InitializedEntity::EK_Variable check. What happens if it's removed? | |
6145 | 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: I am not sure it's correct. | |
6147 | this variable decl can be avoided | |
test/CodeGenOpenCL/opencl_types.cl | ||
39 | Why removing this here? | |
test/SemaOpenCL/invalid-decl.cl | ||
11 | can we merge with some image tests? | |
14 | this can be moved to pipe test? | |
19 | 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! |
include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
593 | I think this no declarator is just for bit-field in C. And OpenCL C does not support the bit-field I think we need not to support that. | |
7666 | It seems that this two are very different and need a big select. Then it seems two maybe OK. | |
lib/Sema/SemaDecl.cpp | ||
5724 | It seems all other check for OpenCL goes this way. I don't know if we just check OpenCL version will cause any error. | |
6718 | agree. I will remove it. | |
lib/Sema/SemaExpr.cpp | ||
6299 | But how can we know if LHS or RHS is Block, or they both are? | |
10095 | Fixed | |
lib/Sema/SemaInit.cpp | ||
6138 | yes, thank you! | |
6147 | I just follow the surrounding code style. | |
test/CodeGenOpenCL/opencl_types.cl | ||
39 | As you can see, it used pointer of image type which I just added a semacheck for it. | |
test/SemaOpenCL/invalid-decl.cl | ||
12 | I will find some place to put these test in. | |
20 | It seems strange to put sema test cases in Parser, just let them be together then. |
include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
593 | I just can't understand the intention here. Could you give any code example or reference to spec? | |
7666 | 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. | |
7670 | 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">; | |
7673 | in OpenCL | |
7674 | 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" | |
7675 | array of block type is ... | |
7677 | in OpenCL | |
lib/Sema/SemaDecl.cpp | ||
5724 | 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? | |
lib/Sema/SemaExpr.cpp | ||
6251 | a block type | |
6299 | 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? | |
6316 | Could you remove this comment? | |
7550 | I am not clear about the purpose of this change. | |
10061 | Block -> block | |
10115 | The code above seems to do similar. Could we combine into one function/diagnostic? | |
lib/Sema/SemaInit.cpp | ||
6139 | 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." | |
6147 | Seems pointless though! | |
6155 | 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. | |
test/CodeGenOpenCL/opencl_types.cl | ||
39 | 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. | |
test/Parser/opencl-atomics-cl20.cl | ||
71 | Why using a macro here? It seems to complicate the test without adding any functionality. | |
test/SemaOpenCL/invalid-decl.cl | ||
6 | 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. |
include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
593 | I will try, | |
7670 | Seems good. But that error message was from spec, this will change it. | |
7674 | OK, it will save few lines. | |
lib/Sema/SemaDecl.cpp | ||
5724 | Yes, but it seems all old code write in that way. I just follow the style. | |
lib/Sema/SemaExpr.cpp | ||
6299 | I don't see any thing talk about the block and condition. Spec only tells about block and selection. | |
6316 | OK. but the "|" may seems strange. I just want to explain it. | |
7550 | That seems some dead experiment. ): | |
10115 | 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. | |
lib/Sema/SemaInit.cpp | ||
6139 | But the example said there can be program scope atomic that only in global address space.
The quote from spec said so. | |
6155 | It seems so. I will have a try. | |
test/CodeGenOpenCL/opencl_types.cl | ||
39 | Thank you advice, if may test for the mangling. I will change them into no-pointer type. | |
test/Parser/opencl-atomics-cl20.cl | ||
71 | Because the the init type maybe different and then have another error. I will have a test for this semacheck as I said above. | |
test/SemaOpenCL/invalid-decl.cl | ||
6 | Keep them be together, and easy to move to right place then. |
include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
7670 | You can pick the wording you like. I personally find this one better! | |
7674 | And more importantly won't populate diagnostics. | |
lib/Sema/SemaDecl.cpp | ||
5724 | Ok, I think an improvement to the old code is always welcome! | |
lib/Sema/SemaExpr.cpp | ||
6299 | Ok, in that case there is nothing left to do here I guess. | |
6316 | Doesn't seem strange to me. | |
10115 | 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? | |
lib/Sema/SemaInit.cpp | ||
6139 | Yes, so copying spec into code is not the goal. Let's try to make it shorter and still have the same meaning! | |
test/Parser/opencl-atomics-cl20.cl | ||
71 | 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): |
Xiuli, do you still plan to continue here?
I was just thinking if it would make sense to re-upload the review since the line numbers got broken due to full diff.
Also it would be nice to partition to several independent commits/reviews. Let's say:
- Blocks diagnostics
- invalid types: images, pipes
- Misc: atomics and implicit declaration
Or you could split the last one into two as well.
What do you think?
include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
593 | Do you still plan to have it? We can discuss in a separate review if you wish to be able to proceed here. |
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!