Page MenuHomePhabricator

__c11_atomic_load's _Atomic can be const
ClosedPublic

Authored by jfb on May 31 2018, 9:55 PM.

Details

Summary

C++11 onwards specs the non-member functions atomic_load and atomic_load_explicit as taking the atomic<T> by const (potentially volatile) pointer. C11, in its infinite wisdom, decided to drop the const, and C17 will fix this with DR459 (the current draft forgot to fix B.16, but that’s not the normative part).

clang’s lib/Headers/stdatomic.h implements these as #define to the __c11_* equivalent, which are builtins with custom typecheck. Fix the typecheck.

D47613 takes care of the libc++ side.

Discussion: http://lists.llvm.org/pipermail/cfe-dev/2018-May/058129.html

rdar://problem/27426936

Diff Detail

Repository
rL LLVM

Event Timeline

jfb created this revision.May 31 2018, 9:55 PM

Note that I don't touch the OpenCL __constant stuff, because the separate address space means this can be actually read-only, which means you can't cmpxchg for very wide reads.

We need to figure out what should happen in the OpenCL case, but the rest seems fine.

lib/Sema/SemaChecking.cpp
3360 ↗(On Diff #149397)

The LoadCopy check is redundant; only the GNU __atomic_load builtin has the LoadCopy form. But see below, we can avoid this duplicated condition with some refactoring.

3361 ↗(On Diff #149397)

We also need to figure out what to do about this -- should an atomic load from a constant address space be valid? (It seems a little pointless to use an *atomic* load here, but not obviously wrong.)

3368–3374 ↗(On Diff #149397)

It would be a little nicer to change this else if to a plain if and conditionalize the diagnostic instead.

Can you track down whoever added the address space check to the C11 atomic path and ask them if they really meant for it to not apply to the GNU atomic builtins?

jfb added a subscriber: yaxunl.Jul 19 2018, 2:57 PM
jfb added inline comments.
lib/Sema/SemaChecking.cpp
3361 ↗(On Diff #149397)

I think it's mostly harmless except when the address space is actually constant (not C++ constant) because in these cases a wide atomic might need cmpxchg to perform a read, and that will fail writing. Maybe @Anastasia can chime in for OpenCL?

3368–3374 ↗(On Diff #149397)

It was @yaxunl in https://reviews.llvm.org/D28691
Nobody asked about GNU atomic builtins with OpenCL in that review. I'll let @yaxunl chime in.

yaxunl added inline comments.Jul 23 2018, 8:27 AM
lib/Sema/SemaChecking.cpp
3368–3374 ↗(On Diff #149397)

Your change LGTM. Originally the check for constant addr space was introduced to match the check of C11 for const. OpenCL itself does not have this restriction.

Anastasia added inline comments.Jul 27 2018, 8:27 AM
test/SemaOpenCL/atomic-ops.cl
61 ↗(On Diff #149397)

Could we add a line with constant AS pointer:

__opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group);

Just to make sure we still give an error for this case.

jfb updated this revision to Diff 157761.Jul 27 2018, 2:01 PM
  • Add constant AS pointer test.
jfb marked an inline comment as done.Jul 27 2018, 2:01 PM

Give the comments, I think this is ready to commit.

Anastasia accepted this revision.Jul 31 2018, 6:34 AM

LGTM from OpenCL side! Thanks!

This revision is now accepted and ready to land.Jul 31 2018, 6:34 AM
This revision was automatically updated to reflect the committed changes.