This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Extended diagnostics for atomic initialization
ClosedPublic

Authored by echuraev on Mar 6 2017, 4:17 AM.

Details

Summary

I saw the same changes in the following review: https://reviews.llvm.org/D17438

I don't know in that way I could determine that atomic variable was initialized by macro ATOMIC_VAR_INIT. Anyway I added check that atomic variables can be initialize only in global scope.
I think that we can discuss this change.

Diff Detail

Event Timeline

echuraev created this revision.Mar 6 2017, 4:17 AM
Anastasia added inline comments.Mar 9 2017, 8:36 AM
include/clang/Basic/DiagnosticSemaKinds.td
8289

Could we combine this error diag with the one below? I guess they are semantically very similar apart from one is about initialization and another is about assignment?

lib/Sema/SemaInit.cpp
6511

I would remove S.getLangOpts().OpenCL check, it's redundant in my opinion!

6514

Even thought it's done above too, I don't really see the point of having this variable.

echuraev updated this revision to Diff 91305.Mar 10 2017, 3:29 AM
echuraev marked 2 inline comments as done.
echuraev added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
8289

I'm not sure that it is a good idea to combine these errors. For example, if developer had declared a variable non-constant and not in global address space he would have got the same message for both errors. And it can be difficult to determine what the exact problem is. He can fix one of the problems but he will still get the same error.

Anastasia added inline comments.Mar 10 2017, 7:18 AM
include/clang/Basic/DiagnosticSemaKinds.td
8289

Well, I don't actually see that we check for constant anywhere so it's also OK if you want to drop this bit. Although I think the original intension of this message as I understood was to provide the most complete hint.

My concern is that these two errors seem to be reporting nearly the same issue and ideally we would like to keep diagnostic list as small as possible. This also makes the file more concise and messages more consistent.

bader added inline comments.Mar 13 2017, 3:26 AM
include/clang/Basic/DiagnosticSemaKinds.td
8289

I suggest adding a test case with non-constant initialization case to validate that existing checks cover this case for atomic types already.
If so, we can adjust existing diagnostic message to cover both cases: initialization and assignment expression.

echuraev updated this revision to Diff 91536.Mar 13 2017, 4:49 AM
bader added inline comments.Mar 13 2017, 5:32 AM
include/clang/Basic/DiagnosticSemaKinds.td
8287–8289

I suggest removing this comment.
If you are going to add other diagnostic messages specific to OpenCL atomics, then separate them from this list of unordered diagnostics similar to pipe built-in functions below.

8289

I don't think it's quite true.
There are two requirements here that must be met the same time. Atomic variables *declared in the global address space* can be initialized only with "compile time constant'.
If understand the spec correctly this code is also valid:

kernel void foo() {
  static global atomic_int a = 42; // although it's not clear if we must use ATOMIC_VAR_INIT here.
  ...
}
Anastasia added inline comments.Mar 13 2017, 10:56 AM
include/clang/Basic/DiagnosticSemaKinds.td
8288

also we should add opencl there:

err_opencl_...

8289

Precisely, but I think checking for compile time constant should be inherited from the general C implementation? I don't think we do anything extra for it. Regarding the macro I am not sure we can suitably diagnose it anyways...

bader added inline comments.Mar 14 2017, 5:51 AM
include/clang/Basic/DiagnosticSemaKinds.td
8289

Precisely, but I think checking for compile time constant should be inherited from the general C implementation?

Agree. I suggested checking this above by extending OpenCL tests, but this can be done separately.

echuraev updated this revision to Diff 93113.Mar 27 2017, 5:14 AM
echuraev marked 2 inline comments as done.
echuraev removed a reviewer: bader.
echuraev added a subscriber: bader.
Anastasia added inline comments.Mar 28 2017, 8:24 AM
test/SemaOpenCL/atomic-init.cl
7

Btw, you could keep "initialized" here by using 'select' in the diagnostic message.

11

Could we also add a check for 'static global' variable?

echuraev updated this revision to Diff 93368.Mar 29 2017, 6:44 AM
echuraev marked 2 inline comments as done.
Anastasia added inline comments.Mar 30 2017, 10:35 AM
test/SemaOpenCL/atomic-init.cl
7

Btw you still keep "assigned" in the error message. What I mean is that we could put "initialized" instead in this case.

echuraev updated this revision to Diff 93602.Mar 31 2017, 1:29 AM
echuraev marked an inline comment as done.
Anastasia added inline comments.Mar 31 2017, 10:22 AM
test/SemaOpenCL/atomic-init.cl
7

This diagnostic seems wrong! How is 1 not a compile time constant?

Didn't we agree to drop the constant bit from the error message and just keep reporting about the program (global visibility) scope? I think the restriction about the scope holds for both initialization and assignment and therefore I have asked to unify the diagnostic...

echuraev updated this revision to Diff 93837.Apr 3 2017, 2:58 AM
echuraev marked an inline comment as done.
Anastasia accepted this revision.Apr 4 2017, 9:12 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Apr 4 2017, 9:12 AM
echuraev closed this revision.Apr 5 2017, 5:59 AM