This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Simplify use of C11 atomic types
ClosedPublic

Authored by Anastasia on Apr 21 2021, 10:18 AM.

Details

Summary

Clang's implementation of the extension pragma is broken because it doesn't respect the following requirement from OpenCL Extension spec s1.2:

https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_Ext.html#extensions-overview

disable Behave (including issuing errors and warnings) as if the extension extension_name is not part of the language definition.

This means that extension functionality should not be exposed by default and if extension identifiers are not reserved they should not be recognized by the compiler. This is for example the case for some of the atomic types (i.e. 64 bit types) from extensions.

As fixing the implementation of extension pragma is not feasible and it will affect backward compatibility and require significant rework in the parser this patch simplifies the implementation of pragma by making the behavior as consistent and intuitive as possible.

This change removes the requirement on pragma when atomic types from the extensions are supported because the behavior is not conformant. With this change, the developers can use atomic types from the extensions if they are supported without enabling the pragma because disabling the pragma doesn't do anything useful but only prevents the use of already available identifiers for types and issues extra diagnostics. This makes semantics consistent with the atomic functions that are also available when the extension is supported without any need for the pragma.

This patch does not break backward compatibility since the extension pragma is still supported and it makes the behavior of the compiler less strict by accepting code without extra pragma statements.

Note that there is an ongoing thread to alter the behavior in the spec
https://github.com/KhronosGroup/OpenCL-Docs/issues/82
however this change is orthogonal as it simplifies the current implementation that is not conformant.
Any new functionality can be added later if the behavior in the spec is fixed.

Diff Detail

Event Timeline

Anastasia created this revision.Apr 21 2021, 10:18 AM
Anastasia requested review of this revision.Apr 21 2021, 10:18 AM
azabaznov added inline comments.Apr 22 2021, 3:51 AM
clang/test/Parser/opencl-atomics-cl20.cl
7–9

Mauby simply -DLANG_VER_OK when running tests?

67–72

Well, this is exactly what I was afraid of last time, see https://reviews.llvm.org/D97058#2602677. Is this for sure a right way to go forward? Also, declared here for implicit type definitions is pretty confusing. Maybe a way the diagnostics showed here is just a bug?

Anastasia added inline comments.Apr 22 2021, 4:53 AM
clang/test/Parser/opencl-atomics-cl20.cl
7–9

I think defining it in the test source is better for future modifications. Then we could add as many extra RUN lines as possible without the need to pass extra -D flag. Also it is kind of redundant since we already have the language version defined while parsing.

67–72

This is a standard diagnostic from clang fixit if the typename is similar to any it can find in a symbol table of valid identifiers.

opencl-atomics-cl20.cl:31:3 error: unknown type name 'atomic_intptr_t'; did you mean 'atomic_int'?
atomic_intptr_t ip;
^~~~~~~~~~~~~~~
atomic_int
note: 'atomic_uint' declared here

For such implicit typedefs we won't be able to display the source locations as there is no physical source. However this can occur in any kernel code compiled with type names similar to those defined by implicit typedefs i.e. I am not changing this in the patch.

https://godbolt.org/z/j7vn6d4Kh

But I agree that this is not ideal - we could solve it by not printing such hints at all if source location doesn't exist but this would probably need to be agreed with the community as this should probably be done for all languages. We could create a clang bug to follow up on this?

This change removes the requirement on pragma when atomic types from the extensions are supported because the behavior is not conformant. With this change, the developers can use atomic types from the extensions if they are supported without enabling the pragma because disabling the pragma doesn't do anything useful but only prevents the use of already available identifiers for types and issues extra diagnostics. This makes semantics consistent with the atomic functions that are also available when the extension is supported without any need for the pragma.

That sounds reasonable to me. Decreasing code complexity and reducing the "surprise-factor" for users by being more consistent is good indeed.

clang/test/Parser/opencl-atomics-cl20.cl
12–14

nit: there are a few tabs in this files you may want to remove before submitting.

50–60

Shouldn't that be

!(defined(cl_khr_int64_extended_atomics) && defined(cl_khr_int64_base_atomics))

for atomic_long and atomic_ulong, and covering double for atomic_double?

Alternatively, if the goal isn't to have 100% coverage in this test of all these variations (which would be fine I believe), then a comment could clarify the intent.

Anastasia added inline comments.May 7 2021, 6:51 AM
clang/test/Parser/opencl-atomics-cl20.cl
50–60

I think for the purpose of this test it is enough that at least one extension is not enabled. So we could add another run line and check for cl_khr_int64_extended_atomics too and also another run line with both although I am not sure it adds a lot into testing at the moment.

We can add a comment, so something like:

Optional type identifiers are not added in earlier version or if at least one of the extensions is not supported. Here we check with `cl_khr_int64_base_atomics` only.
Anastasia added inline comments.May 7 2021, 6:52 AM
clang/test/Parser/opencl-atomics-cl20.cl
50–60

Does it make sense?

mantognini added inline comments.May 7 2021, 6:58 AM
clang/test/Parser/opencl-atomics-cl20.cl
50–60

Yes, I think it does.

Anastasia updated this revision to Diff 344781.May 12 2021, 5:31 AM

Added a comment in the test.

Anastasia marked an inline comment as done.May 12 2021, 5:33 AM
Anastasia added inline comments.
clang/test/Parser/opencl-atomics-cl20.cl
12–14

There were no tabs or anything weird in the test, but now that I have re-created the patch it doesn't show anything. I wonder why it was showing something earlier...

mantognini accepted this revision.May 14 2021, 1:18 AM

LGTM, thanks for the update.

This revision is now accepted and ready to land.May 14 2021, 1:18 AM
This revision was landed with ongoing or failed builds.May 14 2021, 9:43 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2021, 9:43 AM
Herald added a subscriber: ldrumm. · View Herald Transcript