Skip to content

[Clang][AMDGPU] Stop defaulting to one-as for all atomic scopes #120095

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 6, 2025
Merged

[Clang][AMDGPU] Stop defaulting to one-as for all atomic scopes #120095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d4a97c
[Clang][AMDGPU] Stop defaulting to `one-as` for all atomic scopes
jhuber6 Dec 16, 2024
cf2aa30
OpenCL only
jhuber6 Dec 16, 2024
bd723c2
Make test smaller
jhuber6 Dec 16, 2024
04fccb2
Use the scope instead of the language
jhuber6 Dec 17, 2024
6181ebc
add OpenCL test
jhuber6 Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion clang/lib/CodeGen/Targets/AMDGPU.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,11 @@ AMDGPUTargetCodeGenInfo::getLLVMSyncScopeID(const LangOptions &LangOpts,
break;
}

if (Ordering != llvm::AtomicOrdering::SequentiallyConsistent) {
// OpenCL assumes by default that atomic scopes are per-address space for
// non-sequentially consistent operations.
if (Scope >= SyncScope::OpenCLWorkGroup &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain which scopes will generate the one-as versions with this change? I don't know which ones are in that range: agent, workgroup, wavefront, singlethread are the possible candidates.

I also don't understand why the one-as should be generated for a specific range of scopes. How do we know that will not lead to mis-compilation?

Copy link
Contributor Author

@jhuber6 jhuber6 Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenCL ones, this is the range of OpenCL builtins via the opencl_atomic stuff.

Copy link
Contributor

@dhruvachak dhruvachak Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so here's the enum.

enum class SyncScope {
  SystemScope,
  DeviceScope,
  WorkgroupScope,
  WavefrontScope,
  SingleScope,
  HIPSingleThread,
  HIPWavefront,
  HIPWorkgroup,
  HIPAgent,
  HIPSystem,
  OpenCLWorkGroup,
  OpenCLDevice,
  OpenCLAllSVMDevices,
  OpenCLSubGroup,
  Last = OpenCLSubGroup
};

So like you said, the OpenCL ones. If Matt agrees, I am fine with this change though I don't see how it is better than explicitly checking for OpenCL. Just more cryptic. Either way, please add a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a comment though?

Copy link
Contributor Author

@jhuber6 jhuber6 Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better because it's up to the OpenCL builtin, you can still use the 'standard' builtins in OpenCL which would then otherwise give them different behavior depending on the language.

Copy link
Collaborator

@t-tye t-tye Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does seem questionable is that this function is checking the memory ordering. It feels like that should be done by the caller of this function. It is the caller that should be selecting the right value for the scope argument. For OpenCL the *one-as" sync-scopes should only be used for non-sequentially consistent memory orders. That is a rule of the OpenCL language.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the OpenCL builtins outisde of OpenCL https://godbolt.org/z/Ex4aPMx3v. This function is called by all the various reimplementations of this, but with different enum values for the scope. So, if we want the __opencl_ versions to maintain this behavior, we can do that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I am still feeling I am missing something. This PR is making a change to this function. Nothing else. So it seems the question is if the change is correct. This function should only act on the arguments it is given and faithfully do the right thing. It seems that is what it is doing, except for the check of the memory ordering. That check should be in the callers of the function.

Sounds like the callers of this function are the builtins that you describe. The builtins that are intended to implement OpenCL semantics should be sure to pass the O{PENCL* scope values, and should check the memory ordering for being sequentially consistent. The non-OpenCL builtins should pass in the appropriate non-OPENCL values for scope. Is that what is happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, each non-OpenCL function goes through this common interface. The issue is that for non-OpenCL users they all get the one-as feature added, which is not correct in the general case. This patch is an attempt to maintain the OpenCL behavior while the other users of this function will not longer have one-as on them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But with this change that is no longer the case. So is there any push back on this fix?

Scope <= SyncScope::OpenCLSubGroup &&
Ordering != llvm::AtomicOrdering::SequentiallyConsistent) {
if (!Name.empty())
Name = Twine(Twine(Name) + Twine("-")).str();

Expand Down
Loading
Loading