This is an archive of the discontinued LLVM Phabricator instance.

Add OpenCL 2.0 atomic builtin functions as Clang builtin
ClosedPublic

Authored by yaxunl on Jan 13 2017, 12:16 PM.

Details

Summary

OpenCL 2.0 atomic builtin functions have a scope argument which is ideally represented as synchronization scope argument in LLVM atomic instructions.

Clang supports translating Clang atomic builtin functions to LLVM atomic instructions. However it currently does not support synchronization scope of LLVM atomic instructions. Without this, users have to use LLVM assembly code to implement OpenCL atomic builtin functions.

This patch adds OpenCL 2.0 atomic builtin functions as Clang builtin functions, which supports generating LLVM atomic instructions with synchronization scope operand.

Currently only constant memory scope argument is supported. Support of non-constant memory scope argument will be added later.

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 84350.Jan 13 2017, 12:16 PM
yaxunl retitled this revision from to Support synchronisation scope in atomic builtin functions.
yaxunl updated this object.
yaxunl added reviewers: Anastasia, bader, rjmccall.
yaxunl added a subscriber: cfe-commits.
yaxunl updated this revision to Diff 84373.Jan 13 2017, 2:01 PM
yaxunl retitled this revision from Support synchronisation scope in atomic builtin functions to Support synchronisation scope in Clang atomic builtin functions.
yaxunl updated this object.

Fix typos and formatting issues.

rjmccall requested changes to this revision.Jan 17 2017, 8:57 AM

This patch changes the language design of the atomic builtins, which is outside the normal scope of patch review. You need to post an RFC to cfe-dev. I've gone ahead and made some material comments, but the concept itself needs debate.

Your proposed language design exposes LLVM internals (the specific values used by llvm::SynchronizationScope) directly to users; that is also unacceptable.

lib/AST/Expr.cpp
3917

Add an extra newline here, please, to be consistent with the other cases in the switch.

lib/CodeGen/CGAtomic.cpp
116

It is not acceptable to test this exclusively with an assertion; you need to be checking that the argument is an integer constant expression in Sema.

lib/Sema/SemaChecking.cpp
2922

1 is a magic number here.

This revision now requires changes to proceed.Jan 17 2017, 8:57 AM

This patch changes the language design of the atomic builtins, which is outside the normal scope of patch review. You need to post an RFC to cfe-dev. I've gone ahead and made some material comments, but the concept itself needs debate.

Your proposed language design exposes LLVM internals (the specific values used by llvm::SynchronizationScope) directly to users; that is also unacceptable.

I sent an RFC to cfe-dev.

For the synchronization scope, I will add Clang's enums for synchronization scopes in a similar fashion as memory order. Different languages can have their own specific synchronization scopes. Then clang translates them to synchronization scopes used by LLVM atomic instructions.

lib/CodeGen/CGAtomic.cpp
116

Will add diagnostics to Sema.

lib/Sema/SemaChecking.cpp
2922

Will change that to enum.

yaxunl updated this revision to Diff 101955.Jun 8 2017, 1:00 PM
yaxunl edited edge metadata.
yaxunl retitled this revision from Support synchronisation scope in Clang atomic builtin functions to Add OpenCL 2.0 atomic builtin functions as Clang builtin.
yaxunl edited the summary of this revision. (Show Details)

Add __opencl_atomic_ builtins.

Looks like you haven't introduced proper constants in the header for scopes.

docs/LanguageExtensions.rst
1855
  1. "an", not "in".
  2. There's no need to put "explicit" in `` quotes.
  3. The prefix you're using is "__opencl_", with only one trailing underscore.
lib/CodeGen/CGAtomic.cpp
114–1

This is equivalent to checking for the two AO kinds, right? Probably better to just do that.

lib/CodeGen/CGExpr.cpp
1

I think it would be better to keep this function simple and just add the cast outside in the two places you need it.

Also, please remember to use the target hook instead of using an addrspacecast directly.

lib/Sema/SemaChecking.cpp
3046

This clause now covers the scope argument as well.

3060

You need to check that is a constant expression in the correct range, and you should add a test for that.

3062

Please document the meaning of 1.

yaxunl updated this revision to Diff 102374.Jun 13 2017, 11:49 AM
yaxunl marked 11 inline comments as done.
yaxunl edited the summary of this revision. (Show Details)

Revised by John's comments.

Anastasia added inline comments.Jun 20 2017, 9:40 AM
include/clang/Basic/Builtins.def
27

What about min/max? I believe they will need to have the scope too.

include/clang/Basic/DiagnosticSemaKinds.td
6956

Btw, is this disallowed by the spec? Can't find anything relevant.

lib/CodeGen/CGAtomic.cpp
114–1

The same here... not sure adding an extra variable is helping here. :)

115

Seems short enough to introduce an extra variable here. :)

117

Variable name doesn't follow the style.

117

could we avoid using C style cast?

124

Formatting seems to be a bit odd here...

lib/Sema/SemaChecking.cpp
3146

formatting seems odd.

test/CodeGenOpenCL/atomic-ops-libcall.cl
2

GEN4 -> SPIR

3

GEN0 -> AMDGPU

5

Could we use different scopes?

test/CodeGenOpenCL/atomic-ops.cl
8

why do we need this?

16

I think we could use different scope types all over...

33

do we have an addrspacecast before?

test/SemaOpenCL/atomic-ops.cl
23

could we have the full error for consistency?

31

could we also test with constant AS? Also I would generally improve testing wrt address spaces...

yaxunl marked 16 inline comments as done.Jul 25 2017, 7:16 AM
yaxunl added inline comments.
include/clang/Basic/Builtins.def
27

They are not 2.0 atomic builtin functions. They can be implemented as library functions through 2.0 atomic builtin functions.

include/clang/Basic/DiagnosticSemaKinds.td
6956

Just temporarily not supported by Clang. Will add support later.

lib/CodeGen/CGAtomic.cpp
114–1

removed the variable

115

removed the variable

117

will fix

117

will change to static_cast

124

this is done by clang-format

lib/Sema/SemaChecking.cpp
3146

this is done by clang-format

test/CodeGenOpenCL/atomic-ops-libcall.cl
2

will change

3

Actually this triple is armv5e. This test requires a target not supporting atomic instructions. Will change GEN0 -> ARM

5

Yes. will add them.

yaxunl added inline comments.Jul 25 2017, 7:16 AM
test/CodeGenOpenCL/atomic-ops.cl
8

This is to test the builtin works in pch. When generating pch, ALREADY_INCLUDED is undefined, therefore pch will include all functions. When including pch, since ALREADY_INCLUDED is defined through pch, the cl file is empty and function definitions from pch is used for codegen.

16

will add them.

33

No,

test/SemaOpenCL/atomic-ops.cl
23

will do

31

will add tests for different addr spaces.

yaxunl updated this revision to Diff 108073.Jul 25 2017, 7:20 AM
yaxunl marked 16 inline comments as done.
yaxunl edited the summary of this revision. (Show Details)
yaxunl added a reviewer: kzhuravl.

Rebased to ToT and revised by Anastasia's comments.

b-sumner added inline comments.Jul 25 2017, 7:32 AM
include/clang/Basic/Builtins.def
27

Yes, they are. Please look again at 6.13.11.7.5 in the 2.0 C spec.

yaxunl marked 2 inline comments as done.Jul 25 2017, 7:56 AM
yaxunl added inline comments.
include/clang/Basic/Builtins.def
27

sorry I thought they are just some atomic extensions since C++11 atomic builtins do not have those. I will add them.

kzhuravl edited edge metadata.Jul 25 2017, 10:43 AM

Seems like SyncScope.h file is missing?

b-sumner edited edge metadata.Jul 25 2017, 11:32 AM

Can we drop the "opencl" part of the name and use something like __scoped_atomic_*? Also, it may not make sense to support non-constant scope here since we can't predict what other scopes may be added by other languages in the future.

yaxunl marked an inline comment as done.Jul 25 2017, 11:40 AM

Seems like SyncScope.h file is missing?

Right. I will add it.

yaxunl updated this revision to Diff 108127.Jul 25 2017, 11:41 AM

Add min/max and missing file.

Can we drop the "opencl" part of the name and use something like __scoped_atomic_*? Also, it may not make sense to support non-constant scope here since we can't predict what other scopes may be added by other languages in the future.

we could use the approach of LangAS, i.e. we allow targets to map all language specific scopes to target-specific scope names, since IR only cares about scope names, which are target specific. And this is what the current implementation does.

I have no objection to use the __scoped_atomic_ name. It is more general and extensible. John/Anastasia, any comments? Thanks.

Can we drop the "opencl" part of the name and use something like __scoped_atomic_*? Also, it may not make sense to support non-constant scope here since we can't predict what other scopes may be added by other languages in the future.

we could use the approach of LangAS, i.e. we allow targets to map all language specific scopes to target-specific scope names, since IR only cares about scope names, which are target specific. And this is what the current implementation does.

I have no objection to use the __scoped_atomic_ name. It is more general and extensible. John/Anastasia, any comments? Thanks.

I think I would prefer __opencl_atomic_* until we have some evidence that this concept is more general than just OpenCL.

t-tye added a subscriber: t-tye.Jul 25 2017, 12:41 PM

Can we drop the "opencl" part of the name and use something like __scoped_atomic_*? Also, it may not make sense to support non-constant scope here since we can't predict what other scopes may be added by other languages in the future.

we could use the approach of LangAS, i.e. we allow targets to map all language specific scopes to target-specific scope names, since IR only cares about scope names, which are target specific. And this is what the current implementation does.

I have no objection to use the __scoped_atomic_ name. It is more general and extensible. John/Anastasia, any comments? Thanks.

I think I would prefer __opencl_atomic_* until we have some evidence that this concept is more general than just OpenCL.

There are other languages for heterogeneous compute that have scopes, although not exposed quite as explicitly as OpenCL. For example AMD's "HC" language. And any language making use of clang and targeting SPIR-V would likely use these builtins. I think a more generic prefix is appropriate, and "scoped" tells us exactly when these are needed.

There are other languages for heterogeneous compute that have scopes, although not exposed quite as explicitly as OpenCL. For example AMD's "HC" language. And any language making use of clang and targeting SPIR-V would likely use these builtins. I think a more generic prefix is appropriate, and "scoped" tells us exactly when these are needed.

But would those languages use the same language design for these scopes as OpenCL if they did expose them, as opposed to some more elaborate scoping specification? My objection is not that the concept is inherently OpenCL-specific, it's that the presentation in the language might be inherently OpenCL-specific, which makes staying in the opencl namespace is prudent.

There are other languages for heterogeneous compute that have scopes, although not exposed quite as explicitly as OpenCL. For example AMD's "HC" language. And any language making use of clang and targeting SPIR-V would likely use these builtins. I think a more generic prefix is appropriate, and "scoped" tells us exactly when these are needed.

But would those languages use the same language design for these scopes as OpenCL if they did expose them, as opposed to some more elaborate scoping specification? My objection is not that the concept is inherently OpenCL-specific, it's that the presentation in the language might be inherently OpenCL-specific, which makes staying in the opencl namespace is prudent.

Are you envisioning a language far enough from C/C++ that a standard library or header would not be able to map a scoped atomic operation into a call to one of these new builtins? Would we expect more of such languages than languages that would do such a mapping?

There are other languages for heterogeneous compute that have scopes, although not exposed quite as explicitly as OpenCL. For example AMD's "HC" language. And any language making use of clang and targeting SPIR-V would likely use these builtins. I think a more generic prefix is appropriate, and "scoped" tells us exactly when these are needed.

But would those languages use the same language design for these scopes as OpenCL if they did expose them, as opposed to some more elaborate scoping specification? My objection is not that the concept is inherently OpenCL-specific, it's that the presentation in the language might be inherently OpenCL-specific, which makes staying in the opencl namespace is prudent.

Are you envisioning a language far enough from C/C++ that a standard library or header would not be able to map a scoped atomic operation into a call to one of these new builtins? Would we expect more of such languages than languages that would do such a mapping?

If you're using Clang as a frontend for your language, it must be similar enough to C to call a builtin function. That's not at issue. The central question here is whether these builtins are meaningfully general outside of OpenCL. The concept of heterogenous computation is certainly not specific to OpenCL; however, these builtins are defined in terms of scopes — "work item", "work group", "device", and "all svm devices" — which, it seems to me, are basically only defined by reference to the OpenCL architecture. A different heterogenous compute environment might reasonably formalize scopes in a completely different way; for example, it might wish to be more explicit about exactly which peers / devices to synchronize with.

SPIR is explicitly defined on top of the OpenCL model. Users should be able to use OpenCL builtins when targeting it. That does not make those builtins more general than OpenCL.

John.

t-tye added inline comments.Jul 25 2017, 3:53 PM
include/clang/Basic/Builtins.def
30

Will the OpenCL 2.0 memory fences also be supported which also have a memory order and memory scope?

include/clang/Basic/SyncScope.h
26–30

Since the builtins are being named as __opencl then should these also be named as opencl_ since they are the memory scopes for OpenCL using the OpenCL numeric values?

If another language wants to use memory scopes, would it then add its own langx_* names in a similar way that is done for address spaces where the LangAS enumeration type has values for each distinct language. Each target is then responsible for mapping each language scope to the appropriate target specific scope as is done for address spaces?

If so then the builtins are really supporting the concept of memory scopes and are not language specific as this enumeration can support multiple languages in the same way as the LangAS enumeration supports multiple languages. If so the builtins would be better named to reflect this as @b-sumner suggested.

Anastasia edited edge metadata.Jul 28 2017, 3:14 AM

There are other languages for heterogeneous compute that have scopes, although not exposed quite as explicitly as OpenCL. For example AMD's "HC" language. And any language making use of clang and targeting SPIR-V would likely use these builtins. I think a more generic prefix is appropriate, and "scoped" tells us exactly when these are needed.

But would those languages use the same language design for these scopes as OpenCL if they did expose them, as opposed to some more elaborate scoping specification? My objection is not that the concept is inherently OpenCL-specific, it's that the presentation in the language might be inherently OpenCL-specific, which makes staying in the opencl namespace is prudent.

Are you envisioning a language far enough from C/C++ that a standard library or header would not be able to map a scoped atomic operation into a call to one of these new builtins? Would we expect more of such languages than languages that would do such a mapping?

If you're using Clang as a frontend for your language, it must be similar enough to C to call a builtin function. That's not at issue. The central question here is whether these builtins are meaningfully general outside of OpenCL. The concept of heterogenous computation is certainly not specific to OpenCL; however, these builtins are defined in terms of scopes — "work item", "work group", "device", and "all svm devices" — which, it seems to me, are basically only defined by reference to the OpenCL architecture. A different heterogenous compute environment might reasonably formalize scopes in a completely different way; for example, it might wish to be more explicit about exactly which peers / devices to synchronize with.

SPIR is explicitly defined on top of the OpenCL model. Users should be able to use OpenCL builtins when targeting it. That does not make those builtins more general than OpenCL.

John.

The scope concept in OpenCL is fairly generic. And the builtins just take one extra argument on top of the existing C11 builtin style. The OpenCL scopes have of course specific meaning to the OpenCL model, but there is nothing preventing other uses of the scope feature. As far as I understand atomic scope implementation in LLVM is fairly generic wrt scope types and it is intended for broader functionality than just OpenCL. So I would vote for having this as generic as possible in Clang too even though I don't think name prefix __opencl is preventing from using this feature in other languages unless we would disallow the builtins in the other language dialects. Which is not the case with the current patch because the builtins are added as ATOMIC_BUILTIN and not LANGBUILTIN. We have for example used C11 builtin in OpenCL already. So other cases are possible too.

include/clang/Basic/SyncScope.h
26–30

We generally prefix the names of OpenCL specific implementations. So perhaps we should do some renaming here in case we don't intend this to be generic implementation.

lib/Sema/SemaChecking.cpp
3160

Could we merge this and the line after please.

There are other languages for heterogeneous compute that have scopes, although not exposed quite as explicitly as OpenCL. For example AMD's "HC" language. And any language making use of clang and targeting SPIR-V would likely use these builtins. I think a more generic prefix is appropriate, and "scoped" tells us exactly when these are needed.

But would those languages use the same language design for these scopes as OpenCL if they did expose them, as opposed to some more elaborate scoping specification? My objection is not that the concept is inherently OpenCL-specific, it's that the presentation in the language might be inherently OpenCL-specific, which makes staying in the opencl namespace is prudent.

Are you envisioning a language far enough from C/C++ that a standard library or header would not be able to map a scoped atomic operation into a call to one of these new builtins? Would we expect more of such languages than languages that would do such a mapping?

If you're using Clang as a frontend for your language, it must be similar enough to C to call a builtin function. That's not at issue. The central question here is whether these builtins are meaningfully general outside of OpenCL. The concept of heterogenous computation is certainly not specific to OpenCL; however, these builtins are defined in terms of scopes — "work item", "work group", "device", and "all svm devices" — which, it seems to me, are basically only defined by reference to the OpenCL architecture. A different heterogenous compute environment might reasonably formalize scopes in a completely different way; for example, it might wish to be more explicit about exactly which peers / devices to synchronize with.

SPIR is explicitly defined on top of the OpenCL model. Users should be able to use OpenCL builtins when targeting it. That does not make those builtins more general than OpenCL.

John.

The scope concept in OpenCL is fairly generic. And the builtins just take one extra argument on top of the existing C11 builtin style. The OpenCL scopes have of course specific meaning to the OpenCL model, but there is nothing preventing other uses of the scope feature.

Yes, it is possible that some other language could introduce exactly the same scope-atomics concept only with a slightly different enumeration of scopes. But then we really shouldn't allow the OpenCL scopes to be used in that language, which means there would still not be a language-independent way of using these builtins.

As far as I understand atomic scope implementation in LLVM is fairly generic wrt scope types and it is intended for broader functionality than just OpenCL.

In some ways this is reasoning the wrong way around. I am not deeply informed about heterogenous computing, so I am happy to accept that llvm::SynchronizationScope is well-designed for our current needs. But LLVM's representation is, by design, ultimately just an implementation detail and can be easily updated — it's just a matter of changing a few calls and adding an upgrade path to the bitcode loader, exactly as we did when we introduced llvm::SynchronizationScope. That is not true of source language features, even builtins; their design is a contract with programmers, and the bar is substantially higher.

We do not have a compelling reason to claim that these are generally useful, so we should not. They should stay namespaced and be flagged as language-specific builtins.

John.

rjmccall added inline comments.Jul 28 2017, 8:48 AM
include/clang/Basic/SyncScope.h
2

Capitalization.

21

LLVM uses this namespace pattern in code that predates the addition of scoped enums to C++. Those days are behind us; we should just use a scoped enum.

23

It defines the synch scope values used by the atomic builtins and expressions. LLVM's headers define the values used by the instructions.

26–30

I agree that we should name the OpenCL-specific ones, like WorkGroup, with an OpenCL prefix.

33

This is C++; please just use () instead of (void).

lib/CodeGen/CGAtomic.cpp
126

None of the .getTypePtr() stuff here is necessary.

This function shouldn't really be necessary. I would encourage you to add a getValueType() accessor to AtomicExpr:

QualType AtomicExpr::getValueType() const {
  auto T = getPtr()->getType()->castTo<PointerType>()->getPointeeType();
  if (auto AT = T->getAs<AtomicType>()) {
    return AT->getValueType();
  } else {
    return T;
  }
}

You can then just use E->getValueType()->isSignedIntegerType() and eliminate this helper function.

128

Again you're using getTypePtr() unnecessarily.

The check should be whether the AST-level address spaces match, not whether the lowered address spaces match.

Please pass E->getType() instead of E here.

You should remove the DoIt parameter and just check E->isOpenCL() (where E is the AtomicExpr already in scope).

lib/CodeGen/CGCall.cpp
3911 ↗(On Diff #108127)

No. Callers should ensure that they've added the right argument type, at least at the level of address spaces.

lib/CodeGen/TargetInfo.h
8

Why does this return a StringRef instead of an llvm::SynchronizationScope?

yaxunl updated this revision to Diff 109406.Aug 2 2017, 1:31 PM
yaxunl marked 29 inline comments as done.

Revised by reviewers' comments.

t-tye added inline comments.Aug 2 2017, 9:03 PM
include/clang/Basic/SyncScope.h
24

The OpenCL workitem scope is only used for image fences and does not apply to atomic operations so not sure that it should be in this enumeration which is used only for memory atomics.

lib/CodeGen/TargetInfo.cpp
7554–7555

OpenCL only uses workitem for image fences which are not the same as atomic memory fences.

For image fences I don't think it would map to singlethread which is intended for signal handler support to ensure a signal handler has visibility of the updates done by a thread which is more of an optimization barrier. In contrast an image fence may need to flush caches to make the image and vector access paths coherent in a single thread.

Since this patch does not support fences probably want to leave workitem scope out. Current AMDGPU targets do not need to do anything for an OpenCL image fence, but other targets may need to generate an intrinsic since there is no LLVMIR instruction for this.

lib/Headers/opencl-c.h
13143–13151

Do these have to have the same values as the SycnScope enumeration? Should that be ensured in a similar way to the memory_order enumeration?

lib/Sema/SemaChecking.cpp
3103

IIRC OpenCL allows the scope to be a runtime value. So will doing this will likely cause failures in conformance?

The patch generally looks good, but if you need to handle non-constant scopes, you should submit a new patch to address that.

lib/CodeGen/CGAtomic.cpp
896

You can sink this line and the next.

lib/Sema/SemaChecking.cpp
3103

Ah, if that's true, you'll need to emit a switch in IRGen, the same way we handle non-constant memory orders.

yaxunl marked 6 inline comments as done.Aug 3 2017, 6:30 AM
yaxunl added inline comments.
include/clang/Basic/Builtins.def
30

I am considering supporting it with a separate patch since this patch is already quite large.

include/clang/Basic/SyncScope.h
24

You are right. I think we should drop it from this enum for now.

lib/CodeGen/CGAtomic.cpp
896

will do.

lib/CodeGen/CGCall.cpp
3911 ↗(On Diff #108127)

Will remove.

lib/CodeGen/TargetInfo.cpp
7554–7555

Thanks. I will remove this for now.

lib/Headers/opencl-c.h
13143–13151

It is desirable to have the same value as SyncScope enumeration, otherwise the library has to do the translation when calling __opencl_atomic_* builtins.

Will do.

lib/Sema/SemaChecking.cpp
3103

Will support it in another patch since this one is already quite large.

3160

will do

yaxunl updated this revision to Diff 109542.Aug 3 2017, 6:41 AM
yaxunl marked 5 inline comments as done.

Revised by Tony's and John's comments.

yaxunl updated this revision to Diff 109556.Aug 3 2017, 7:27 AM

Add assert to make sure pre-defined macros __OPENCL_MEMORY_SCOP_* to be consistent with SyncScope enum.

LGTM. I'm fine with the plan to handle potentially non-constant scopes in a follow-up patch.

include/clang/Basic/SyncScope.h
21

If the numeric values here were chosen to align with the arguments to some runtime function, that's important to leave as a comment.

lib/Headers/opencl-c.h
13143–13151

Since we're defining these builtins ourselves de novo, it's fine to pick argument values that align with what the existing runtime functions expect. Once the builtins are defined and in-use, of course, we cannot subsequently change the builtin values, even if the runtime changes.

yaxunl updated this revision to Diff 109591.Aug 3 2017, 10:30 AM

Add comments to SyncScope.h

t-tye added inline comments.Aug 3 2017, 11:29 AM
docs/LanguageExtensions.rst
1935

Should it also say:

The macros ``__OPENCL_MEMORY_SCOPE_WORK_ITEM``, ``__OPENCL_MEMORY_SCOPE_WORK_GROUP``, ``__OPENCL_MEMORY_SCOPE_DEVICE``, ``__OPENCL_MEMORY_SCOPE_ALL_SVM_DEVICES``, and ``__OPENCL_MEMORY_SCOPE_SUB_GROUP`` are provided, with values corresponding to the enumerators of OpenCL's ``memory_scope`` enumeration.
yaxunl marked an inline comment as done.Aug 3 2017, 12:12 PM
yaxunl added inline comments.
docs/LanguageExtensions.rst
1935

Thanks. Will do.

yaxunl updated this revision to Diff 109612.Aug 3 2017, 12:14 PM
yaxunl marked an inline comment as done.

Added documentation about __OPENCL_MEMORY_SCOPE_* by Tony's comments.

t-tye accepted this revision.Aug 3 2017, 1:54 PM

LGTM

yaxunl marked 10 inline comments as done.Aug 4 2017, 6:25 AM
yaxunl added inline comments.
include/clang/Basic/SyncScope.h
21

I have added the comments and updated the review.. On the Phabricator web page, it may take a bit effort to find it since it is interleaved with reviewers' comments.

bader accepted this revision.Aug 4 2017, 8:11 AM
bader added inline comments.
lib/AST/Expr.cpp
4000–4004

No need in else branch after return:

if (...) {
  return AT->getValueType();
}

return T;

http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

test/SemaOpenCL/atomic-ops.cl
2

It's a pity, we have to parse the whole opencl-c.h file to get two enums and one typedef...

yaxunl marked 3 inline comments as done.Aug 4 2017, 8:33 AM

Ping. Any other comments? Thanks.

lib/AST/Expr.cpp
4000–4004

will fix when committing.

test/SemaOpenCL/atomic-ops.cl
2

Since there is change in opencl-c.h, using the header can test that.

rjmccall accepted this revision.Aug 4 2017, 10:01 AM

Still LGTM.

This revision is now accepted and ready to land.Aug 4 2017, 10:01 AM
This revision was automatically updated to reflect the committed changes.
yaxunl marked 2 inline comments as done.