This is an archive of the discontinued LLVM Phabricator instance.

[hip] Add HIP scope atomic ops.
AbandonedPublic

Authored by hliao on Sep 17 2020, 1:59 PM.

Diff Detail

Event Timeline

hliao created this revision.Sep 17 2020, 1:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2020, 1:59 PM
hliao requested review of this revision.Sep 17 2020, 1:59 PM
hliao added a comment.Sep 17 2020, 2:04 PM

singlethread scope is added for completeness for the atomicity considering signal handling. It may not be applicable to GPU yet. We could simply ignore them in the late phases of the compiler. If desired, I may remove it as well.

hliao updated this revision to Diff 292622.Sep 17 2020, 2:37 PM

Revise formatting following the clang-format suggestion.

t-tye added a subscriber: t-tye.Sep 17 2020, 2:39 PM
t-tye added inline comments.Sep 17 2020, 2:55 PM
clang/include/clang/Basic/Builtins.def
792–799

Should HIP also consider adding a hip_atomic_load and hip_atomic_store?

clang/lib/CodeGen/TargetInfo.cpp
9161–9179

The HIP memory model uses a single happens-before relation for all address spaces which is different to OpenCL. So these need to use the "*-one-as" names.

Before committing this we should decide if we want to implement the OpenCL address space fence support using the syncScope, in which case we may want to adjust this name before we start using it.

clang/lib/Sema/SemaChecking.cpp
4877

Does HIP have atomic types for this to be meaningful? I would expect that HIP should be treated the same as OpenCL which does not appear to be here.

t-tye requested changes to this revision.Sep 17 2020, 2:55 PM
This revision now requires changes to proceed.Sep 17 2020, 2:55 PM
jfb added a comment.Sep 17 2020, 3:04 PM

Please provide documentation in this patch.

t-tye added inline comments.Sep 17 2020, 4:33 PM
clang/include/clang/Basic/Builtins.def
792–799

What is being done for the memory fences?

Does HIP have any interest in adding a memory order?

clang/lib/CodeGen/TargetInfo.cpp
9161–9179

Actually the other way round, OpenCL should be using the "*-one-as" names except for the seq_cst memory ordering which is all address spaces.

In D87858#2280429, @jfb wrote:

Please provide documentation in this patch.

opencl atomic builtins are documented as notes to __c11_atomic builtins part of https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions. These new atomic builtins can be documented in a similar way after that.

jfb added a comment.Sep 18 2020, 9:52 AM
In D87858#2280429, @jfb wrote:

Please provide documentation in this patch.

opencl atomic builtins are documented as notes to __c11_atomic builtins part of https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions. These new atomic builtins can be documented in a similar way after that.

I'd like to see the documentation added to this patch.

ashi1 added a subscriber: ashi1.Oct 13 2020, 10:23 AM
hliao added a comment.Jan 25 2021, 8:05 AM

PING for review.

Hi Michael, would you like to continue working on this, or let someone from AMD to take over? Thanks.

hliao added a comment.Nov 11 2021, 1:22 PM

Hi Michael, would you like to continue working on this, or let someone from AMD to take over? Thanks.

please take over it. I will abandon it so that you could start it over.

hliao abandoned this revision.Nov 11 2021, 1:22 PM