Page MenuHomePhabricator

[OpenCL] Add Sema checks for OpenCL 2.0
AbandonedPublic

Authored by pxli168 on Jan 11 2016, 1:02 AM.

Details

Summary

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
lib/Sema/SemaDecl.cpp
5733

Is this intentionally included in the patch?

6759

Ditto. Better not commit disabled code in the repository.

7305

Ditto.

lib/Sema/SemaExpr.cpp
6295

-28

pxli168 added inline comments.Jan 11 2016, 8:47 PM
lib/Sema/SemaDecl.cpp
5733

My mistake, just want to check if this works. But find it is handled by something else.

6759

Removed

lib/Sema/SemaExpr.cpp
6295

Fixed

6298

Intend to do this in order to get both err diag for both LHS and RHS.

Remove some unused codes and add inline comment.

Anastasia edited edge metadata.Jan 12 2016, 2:58 AM

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:
kernel void foo(){ global int x;}

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!

pxli168 added inline comments.Jan 12 2016, 4:36 AM
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.
If you think it is useless I think we can remove it then.

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.

pxli168 edited edge metadata.

FIxed bugs and relocate the test cases.
Upload the full diff.

Anastasia added inline comments.Jan 13 2016, 3:58 AM
include/clang/Basic/DiagnosticSemaKinds.td
600

I just can't understand the intention here. Could you give any code example or reference to spec?

7700

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.

7704

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">;
7707

in OpenCL

7708

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"

7709

array of block type is ...

7711

in OpenCL

lib/Sema/SemaDecl.cpp
5739

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
6429

a block type

6477

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?

6494

Could you remove this comment?

7727

I am not clear about the purpose of this change.

10239

Block -> block

10293

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
7

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.

pxli168 added inline comments.Jan 14 2016, 8:19 AM
include/clang/Basic/DiagnosticSemaKinds.td
600

I will try,

7704

Seems good. But that error message was from spec, this will change it.

7708

OK, it will save few lines.

lib/Sema/SemaDecl.cpp
5739

Yes, but it seems all old code write in that way. I just follow the style.

lib/Sema/SemaExpr.cpp
6477

I don't see any thing talk about the block and condition. Spec only tells about block and selection.

6494

OK. but the "|" may seems strange. I just want to explain it.

7727

That seems some dead experiment. ):

10293

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.

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.

6155

It seems so. I will have a try.

test/CodeGenOpenCL/opencl_types.cl
39

Thank you advice, if may test for the mangling.
But the name bad make me the think it is a useless test so I just remove it.

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
7

Keep them be together, and easy to move to right place then.

Anastasia added inline comments.Jan 14 2016, 10:54 AM
include/clang/Basic/DiagnosticSemaKinds.td
7704

You can pick the wording you like. I personally find this one better!

7708

And more importantly won't populate diagnostics.

lib/Sema/SemaDecl.cpp
5739

Ok, I think an improvement to the old code is always welcome!

lib/Sema/SemaExpr.cpp
6477

Ok, in that case there is nothing left to do here I guess.

6494

Doesn't seem strange to me.

10293

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):
atomic_int guide = 42;

Anastasia added inline comments.Jan 18 2016, 6:15 AM
lib/Sema/SemaDecl.cpp
7279–7285

I think this should be removed?

7303

Is there a test for this?

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
600

Do you still plan to have it? We can discuss in a separate review if you wish to be able to proceed here.

pxli168 marked 6 inline comments as done.Jan 26 2016, 8:22 PM

I will separate this patch into small ones.

lib/Sema/SemaDecl.cpp
5739

OK, I think we can try here to see if it will bring some errors.

7303

I will add one.

pxli168 updated this object.

Rebase for partition

pxli168 abandoned this revision.Feb 21 2016, 10:40 PM