Skip to content

[spirv][amdgpu] Set atomic size in the clang target info #128569

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.

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 1 commit into from
Feb 25, 2025

Conversation

JonChesterfield
Copy link
Collaborator

Problem identified by Joseph. The openmp device runtime uses __scoped_atomic_load_n and similar which presently hit

error: large atomic operation may incur significant performance
      penalty; the access size (4 bytes) exceeds the max lock-free size (0 bytes) [-Werror,-Watomic-alignment]

This is because the spirv class doesn't set the corresponding field. The base does, but only if there's a host toolchain, which there isn't.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 24, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 24, 2025

@llvm/pr-subscribers-clang

Author: Jon Chesterfield (JonChesterfield)

Changes

Problem identified by Joseph. The openmp device runtime uses __scoped_atomic_load_n and similar which presently hit

error: large atomic operation may incur significant performance
      penalty; the access size (4 bytes) exceeds the max lock-free size (0 bytes) [-Werror,-Watomic-alignment]

This is because the spirv class doesn't set the corresponding field. The base does, but only if there's a host toolchain, which there isn't.


Full diff: https://github.com/llvm/llvm-project/pull/128569.diff

1 Files Affected:

  • (modified) clang/lib/Basic/Targets/SPIR.h (+2)
diff --git a/clang/lib/Basic/Targets/SPIR.h b/clang/lib/Basic/Targets/SPIR.h
index 61f9ef7e3e361..610efa1fe00d9 100644
--- a/clang/lib/Basic/Targets/SPIR.h
+++ b/clang/lib/Basic/Targets/SPIR.h
@@ -399,6 +399,8 @@ class LLVM_LIBRARY_VISIBILITY SPIRV64AMDGCNTargetInfo final
     HasLegalHalfType = true;
     HasFloat16 = true;
     HalfArgsAndReturns = true;
+
+    MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
   }
 
   bool hasBFloat16Type() const override { return true; }

Copy link
Contributor

@AlexVlx AlexVlx left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. FWIW we've only done HIP bring-up, so there will probably be a few of these lingering.

@JonChesterfield JonChesterfield merged commit 43999de into llvm:main Feb 25, 2025
14 checks passed
@JonChesterfield JonChesterfield deleted the jc_spirv_atomic_size branch February 25, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants