Page MenuHomePhabricator

[OpenCL] Support variable memory scope in atomic builtins
ClosedPublic

Authored by yaxunl on Aug 10 2017, 7:09 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl created this revision.Aug 10 2017, 7:09 AM
rjmccall added inline comments.Aug 11 2017, 10:54 AM
include/clang/Basic/SyncScope.h
47 ↗(On Diff #110575)

You could just return an ArrayRef to a static const array.

lib/CodeGen/CGAtomic.cpp
687 ↗(On Diff #110575)

Does Sema not coerce the argument to int? It really should, and then you can just rely on that here. (You should use CGF.IntTy if you do this, though, in case you're on a target with a non-32-bit int.)

t-tye added inline comments.Aug 11 2017, 1:43 PM
include/clang/Basic/SyncScope.h
46 ↗(On Diff #110575)

Should there be an assert/static_assert in case SyncScope enum grows due to other languages?

59 ↗(On Diff #110575)

Should there be a default/assert/static_assert to allow SyncScope enum to grow due to other languages?

lib/CodeGen/CGAtomic.cpp
696 ↗(On Diff #110575)

Is it documented in the SyncScope enum that the enumerator values are in fact the values used for source language runtime values? Seems if other languages want to use scopes they may may have a different ordering. That would imply that there would be a function to map a SyncScope value to the value used by the source language. For OpenCL the mapping is identity.

The memory ordering has the isValidAtomicOrderingCABI() that does a similar thing.

rjmccall added inline comments.Aug 11 2017, 2:26 PM
include/clang/Basic/SyncScope.h
46 ↗(On Diff #110575)

It makes sense to add a 'last' enumerator to SyncScope and do a static_assert here that last == OpenCLSubGroup.

59 ↗(On Diff #110575)

There will already be a warning from -Wswitch about this, which should be sufficient.

But we do often add llvm_unreachable after switches like this.

lib/CodeGen/CGAtomic.cpp
696 ↗(On Diff #110575)

The values in the SyncScope enum are the source language values. We already have a step to translate them into LLVM values when we generate a native LLVM construct. To the extent that we call into a runtime instead, none of that code has been written to be runtime-agnostic at all, and I've been assuming that we're basically okay with that, at least for now.

t-tye accepted this revision.Aug 11 2017, 3:50 PM

LGTM other than suggested documentation and static_assert/unreachable comments.

lib/CodeGen/CGAtomic.cpp
696 ↗(On Diff #110575)

That sounds reasonable to me. When/if another language comes along the task of mapping the multiple ABIs can be done then. Still wanted to make sure it is clear that the enum values in the SyncScope enum are documented as BEING the OpenCL runtime ABI values and not some arbitrary list as in other enums such as the address space enum.

This revision is now accepted and ready to land.Aug 11 2017, 3:50 PM
rjmccall added inline comments.Aug 11 2017, 5:14 PM
lib/CodeGen/CGAtomic.cpp
696 ↗(On Diff #110575)

They are documented as being the values expected by the builtins, so they already can't be arbitrarily changed.

Now, the values expected by the builtin were chosen to match this specific runtime, but we had to choose something, and matching a runtime isn't the worst way of doing that. But now that I think about it, those values seem to be rather sub-optimal because they don't start at 0, which means that things like this switch will be less efficient than they could be. And I think the AST and IRGen would benefit from needing to renumber values; it will make the code clearer, and it'll help protect against people adding values to this enum just because those values are used by the runtime, i.e. without fully realizing that they're expanding a user-facing feature. So let's go ahead and renumber the values in SyncScope to start at 0 and then re-map them in IRGen.

yaxunl marked 10 inline comments as done.Aug 12 2017, 9:34 PM
yaxunl added inline comments.
include/clang/Basic/SyncScope.h
46 ↗(On Diff #110575)

Thanks. Will do.

47 ↗(On Diff #110575)

Thanks. Will do.

59 ↗(On Diff #110575)

I tried adding

default:
    llvm_unreachable("Invalid sync scope");

However, it results in error:

error: default label in switch which covers all enumeration values [-Werror,-Wcovered-switch-default]

I guess I'd better leave it as it is since we already have -Wswitch.

lib/CodeGen/CGAtomic.cpp
687 ↗(On Diff #110575)

It is coerced to Context.IntTy. I will add some tests for non-integral order/scope argument.

696 ↗(On Diff #110575)

Will do. Thanks.

yaxunl updated this revision to Diff 110863.Aug 12 2017, 10:01 PM
yaxunl marked 5 inline comments as done.

Revised by John's and Tony's comments.

t-tye added inline comments.Aug 13 2017, 2:44 AM
include/clang/Basic/SyncScope.h
56 ↗(On Diff #110863)

AtomicScopeOpenCLABI to mirror AtomicOrderingCABI?

94 ↗(On Diff #110863)

Should this take a LangOpt since different languages may use different value ABIs?

lib/CodeGen/CGAtomic.cpp
678 ↗(On Diff #110863)

Should only the scopes that apply to the language be returned otherwise will be generating code for invalid (possibly duplicate ABI) values?

lib/Frontend/InitPreprocessor.cpp
582 ↗(On Diff #110863)

Could this be static_assert?

yaxunl marked 8 inline comments as done.Aug 13 2017, 6:57 AM
yaxunl added inline comments.
include/clang/Basic/SyncScope.h
56 ↗(On Diff #110863)

Will do.

94 ↗(On Diff #110863)

Although currently this function does not use LangOpt, but I agree it may be a good idea to make it future proof. Will add it and update comment.

lib/CodeGen/CGAtomic.cpp
678 ↗(On Diff #110863)

getAllLanguageSyncScopes() only returns scope values for current language. I will rename it to getRuntimeSyncScopeValuesForCurrentLanguage() to avoid confusing.

t-tye added inline comments.Aug 13 2017, 11:44 AM
lib/CodeGen/CGAtomic.cpp
678 ↗(On Diff #110863)

Curretly getAllLanguageSyncScopes does not take a LangOpt so not sure how it knows what the language is, and did not see it checking that the language is OpenCL internally. For non-OpenCL languages do they still have to support system scope?

yaxunl marked 6 inline comments as done.Aug 13 2017, 8:01 PM
yaxunl added inline comments.
lib/CodeGen/CGAtomic.cpp
678 ↗(On Diff #110863)

For now I just assume all languages use OpenCL memory scope ABI. If a language has its own memory scope ABI it can be added later.

lib/Frontend/InitPreprocessor.cpp
582 ↗(On Diff #110863)

Will do.

yaxunl updated this revision to Diff 110904.Aug 13 2017, 8:51 PM
yaxunl marked 2 inline comments as done.

Refactor to introduce classes AtomicScopeABI and AtomicScopeOpenCLABI for easy extension to other languages.

t-tye accepted this revision.Aug 13 2017, 9:07 PM

LGTM

rjmccall added inline comments.Aug 14 2017, 10:43 AM
include/clang/Basic/SyncScope.h
59 ↗(On Diff #110575)

Yeah, the idiom is to put the llvm_unreachable after the switch, not in a default. Check out the uses in Type.cpp for examples: some of them are in specific cases, saying those cases are illlegal due to some precondition, but most of them are just after the switch saying that the switch is covered. (The semantics of switch in C are that switch values with no corresponding case just skip the whole switch unless there's a default.)

89 ↗(On Diff #110904)

The previous design of this header seems more appropriate to me. Clang defines a builtin with standardized argument values for all targets; we're not trying to allow targets to customize the arguments taken by this builtin, at least not yet. The sync-scope argument values are the values from this enum. Because they are potentially exposed in source programs, they cannot be lightly changed and re-ordered; that is, they are ABI. There is a second level of ABI, which is the set of values demanded by target runtime functions. It is IRGen's responsibility to turn the SyncScope values into those values when generating a call to the runtime; that mapping does not need to be exposed here in the AST.

92 ↗(On Diff #110904)

Please correct me if I'm wrong, but I believe you don't get to define the "ABI for OpenCL" any more than I get to define the "ABI for C". You're defining the ABI for your specific OpenCL implementation, but Clang might reasonably support other implementations with their own ABIs. So this name is inappropriate.

Now, if I understand how SPIR works, SPIR does require some sort of stable lowering of these atomic operations in order to ensure that the resulting LLVM IR can be ingested into an arbitrary implementation. (Ot at least that was true prior to SPIR V?) Therefore, SPIR needs to specify a lowering for these atomic operations, and that probably includes ABI values for specific sync-scopes. But the appropriate name for that would be the "SPIR ABI", not the "OpenCL ABI". And, of course, you would need to be sure that you're implementing the lowering that SPIR actually wants.

yaxunl marked 7 inline comments as done.Aug 14 2017, 11:00 AM
yaxunl added inline comments.
include/clang/Basic/SyncScope.h
89 ↗(On Diff #110904)

This function is not intended to create ABI's for different targets, but to create ABI's for different languages. Currently only OpenCL supports atomic scope, but in the future there may be other languages which support atomic scope with a different ABI.

92 ↗(On Diff #110904)

The ABI is not intended for a specific OpenCL implementation, but for extending to other languages. For example, we have a downstream C++ based language called HCC, which may support atomic scope with an ABI different than OpenCL atomic scope ABI.

rjmccall added inline comments.Aug 14 2017, 11:21 AM
include/clang/Basic/SyncScope.h
92 ↗(On Diff #110904)

By "ABI" you mean that it might present a different model of synchronization scopes to the source language? Okay, that's great, and it explains a lot of things that were very murky about some of our previous conversations.

In that case, I think the appropriate design for these builtins is:

  • They don't need to be in the __builtin_opencl namespace.
  • They *do* need to be enabled only in language modes with well-defined sync-scope models, which for now just means OpenCL.
  • The language mode's sync-scope model should determine everything about the scope argument, including its type. Sema-level validation requires first determining the language's sync-scope model.
  • There is no meaningful way to "default" the scope argument on the non-scoped builtins the way that we are now. Instead, the scope argument should only exist for the scoped builtins.

Alternatively, if you potentially want the opencl-model builtins to be usable from non-opencl languages, the right design is for HCC to use its own set of builtins with their own type-checking rules.

In either case, I don't think it's a good idea to call this "ABI", which is associated too strongly with target-lowering concepts. You're really talking about a source-language concept.

yaxunl marked 4 inline comments as done.Aug 14 2017, 11:43 AM
yaxunl added inline comments.
include/clang/Basic/SyncScope.h
92 ↗(On Diff #110904)

We could introduce __builtin_hcc_atomic_* in downstream HCC.

We need a generic approach to represent atomic scope values for different languages and associated semantic/codegen needs, and AtomicScopeABI seems generic enough to serve the purpose. It has interface for semantic checking (isValid), and the interface to map to LLVM sync scope for both constant and variable arguments. If the name is not proper, we could rename it to just AtomicScope.

I can remove the default atomic scope for atomic expressions without scope originally. For them I just emit LLVM atomic instructions with default LLVM synch scope.

rjmccall added inline comments.Aug 14 2017, 12:11 PM
include/clang/Basic/SyncScope.h
92 ↗(On Diff #110904)

I agree that there's value in having a generic approach even if you're going to have different builtins for different scope models. However, if you do use different builtins for different scope models, you need to make sure that you pick the appropriate model for the builtin being used, not according to the current language mode. That is, there should be some way to map a builtin ID to an AtomicScopeModelKind (values: None, OpenCL, (future) HCC), and then you can use that to create an AtomicScopeModel, which can have all these methods on it.

I would encourage you to go ahead and give AtomicScopeModel an implementation file and move more of these method definitions into that.

105 ↗(On Diff #110904)

I would call this OpenCLAtomicScopeModel.

128 ↗(On Diff #110904)

Make this const, please.

yaxunl marked 6 inline comments as done.Aug 14 2017, 9:27 PM
yaxunl added inline comments.
include/clang/Basic/SyncScope.h
92 ↗(On Diff #110904)

Thanks. Will do.

105 ↗(On Diff #110904)

Will do.

128 ↗(On Diff #110904)

will do.

yaxunl updated this revision to Diff 111160.Aug 15 2017, 7:12 AM
yaxunl marked 3 inline comments as done.

Revised by John's comments.

rjmccall accepted this revision.Aug 15 2017, 7:46 AM

Thanks, that looks great.

This revision was automatically updated to reflect the committed changes.
t-tye added inline comments.Aug 15 2017, 4:26 PM
cfe/trunk/lib/Frontend/InitPreprocessor.cpp
581

// The values should match clang AtomicScopeOpenCLModel::ID enum.

yaxunl marked an inline comment as done.Aug 15 2017, 9:16 PM
yaxunl added inline comments.
cfe/trunk/lib/Frontend/InitPreprocessor.cpp
581

Fixed. Thanks.