[SPIR-V] Honor explicit memory_order/memory_scope in atomic load/store by aobolensk · Pull Request #215572 · llvm/llvm-project

aobolensk · 2026-08-11T14:04:49Z

atomic_load_explicit and atomic_store_explicit ignored their memory_order/memory_scope arguments

Translate the explicit arguments via the existing buildMemSemanticsReg/buildScopeReg helpers, as buildAtomicRMWInst already does

atomic_load_explicit and atomic_store_explicit ignored their memory_order/memory_scope arguments Translate the explicit arguments via the existing buildMemSemanticsReg/buildScopeReg helpers, as buildAtomicRMWInst already does

llvmorg-github-actions · 2026-08-11T14:05:45Z

@llvm/pr-subscribers-backend-spir-v

Author: Arseniy Obolenskiy (aobolensk)

Changes

atomic_load_explicit and atomic_store_explicit ignored their memory_order/memory_scope arguments

Translate the explicit arguments via the existing buildMemSemanticsReg/buildScopeReg helpers, as buildAtomicRMWInst already does

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

2 Files Affected:

(modified) llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp (+24-20)
(modified) llvm/test/CodeGen/SPIRV/AtomicBuiltinsFloat.ll (+12-3)

diff --git a/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp b/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp
index 89b0292bc656b..38c770aed5836 100644
--- a/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp
@@ -635,24 +635,22 @@ static bool buildAtomicLoadInst(const SPIRV::IncomingCall *Call,
   if (Call->isSpirvOp())
     return buildOpFromWrapper(MIRBuilder, SPIRV::OpAtomicLoad, Call, TypeReg);
 
+  // atomic_load_explicit(ptr, memory_order[, memory_scope]).
   Register PtrRegister = Call->Arguments[0];
-  // TODO: if true insert call to __translate_ocl_memory_sccope before
-  // OpAtomicLoad and the function implementation. We can use Translator's
-  // output for transcoding/atomic_explicit_arguments.cl as an example.
+  MachineRegisterInfo *MRI = MIRBuilder.getMRI();
+
+  unsigned Semantics =
+      SPIRV::MemorySemantics::SequentiallyConsistent |
+      getMemSemanticsForStorageClass(GR->getPointerStorageClass(PtrRegister));
+  Register MemSemanticsReg =
+      Call->Arguments.size() >= 2 ? Call->Arguments[1] : Register();
+  MemSemanticsReg = buildMemSemanticsReg(MemSemanticsReg, PtrRegister,
+                                         Semantics, MIRBuilder, GR);
+
   Register ScopeRegister =
-      Call->Arguments.size() > 1
-          ? Call->Arguments[1]
-          : buildConstantIntReg32(SPIRV::Scope::Device, MIRBuilder, GR);
-  Register MemSemanticsReg;
-  if (Call->Arguments.size() > 2) {
-    // TODO: Insert call to __translate_ocl_memory_order before OpAtomicLoad.
-    MemSemanticsReg = Call->Arguments[2];
-  } else {
-    int Semantics =
-        SPIRV::MemorySemantics::SequentiallyConsistent |
-        getMemSemanticsForStorageClass(GR->getPointerStorageClass(PtrRegister));
-    MemSemanticsReg = buildConstantIntReg32(Semantics, MIRBuilder, GR);
-  }
+      Call->Arguments.size() >= 3 ? Call->Arguments[2] : Register();
+  ScopeRegister =
+      buildScopeReg(ScopeRegister, SPIRV::Scope::Device, MIRBuilder, GR, MRI);
 
   MIRBuilder.buildInstr(SPIRV::OpAtomicLoad)
       .addDef(Call->ReturnRegister)
@@ -671,13 +669,19 @@ static bool buildAtomicStoreInst(const SPIRV::IncomingCall *Call,
     return buildOpFromWrapper(MIRBuilder, SPIRV::OpAtomicStore, Call,
                               Register(0));
 
-  Register ScopeRegister =
-      buildConstantIntReg32(SPIRV::Scope::Device, MIRBuilder, GR);
+  MachineRegisterInfo *MRI = MIRBuilder.getMRI();
   Register PtrRegister = Call->Arguments[0];
-  int Semantics =
+  unsigned Semantics =
       SPIRV::MemorySemantics::SequentiallyConsistent |
       getMemSemanticsForStorageClass(GR->getPointerStorageClass(PtrRegister));
-  Register MemSemanticsReg = buildConstantIntReg32(Semantics, MIRBuilder, GR);
+  Register MemSemanticsReg =
+      Call->Arguments.size() >= 3 ? Call->Arguments[2] : Register();
+  MemSemanticsReg = buildMemSemanticsReg(MemSemanticsReg, PtrRegister,
+                                         Semantics, MIRBuilder, GR);
+  Register ScopeRegister =
+      Call->Arguments.size() >= 4 ? Call->Arguments[3] : Register();
+  ScopeRegister =
+      buildScopeReg(ScopeRegister, SPIRV::Scope::Device, MIRBuilder, GR, MRI);
   MIRBuilder.buildInstr(SPIRV::OpAtomicStore)
       .addUse(PtrRegister)
       .addUse(ScopeRegister)
diff --git a/llvm/test/CodeGen/SPIRV/AtomicBuiltinsFloat.ll b/llvm/test/CodeGen/SPIRV/AtomicBuiltinsFloat.ll
index cb81df5b61019..4c41fe3c92694 100644
--- a/llvm/test/CodeGen/SPIRV/AtomicBuiltinsFloat.ll
+++ b/llvm/test/CodeGen/SPIRV/AtomicBuiltinsFloat.ll
@@ -3,12 +3,21 @@
 
 ;; Types:
 ; CHECK:         %[[#F32:]] = OpTypeFloat 32
+; CHECK:         %[[#I32:]] = OpTypeInt 32 0
 ;; Constants:
-; CHECK:         %[[#CONST:]] = OpConstant %[[#F32]] 1
+; CHECK-DAG:     %[[#CONST:]] = OpConstant %[[#F32]] 1
+; CHECK-DAG:     %[[#RELAXED:]] = OpConstantNull %[[#I32]]
+; CHECK-DAG:     %[[#DEVICE:]] = OpConstant %[[#I32]] 1
+; CHECK-DAG:     %[[#WORKGROUP:]] = OpConstant %[[#I32]] 2
+; CHECK-DAG:     %[[#SEQCST:]] = OpConstant %[[#I32]] 16
 ;; Atomic instructions:
 ; CHECK:         OpStore %[[#]] %[[#CONST]]
-; CHECK-COUNT-3: OpAtomicStore
-; CHECK-COUNT-3: OpAtomicLoad
+; CHECK:         OpAtomicStore %[[#]] %[[#DEVICE]] %[[#SEQCST]] %[[#CONST]]
+; CHECK:         OpAtomicStore %[[#]] %[[#DEVICE]] %[[#RELAXED]] %[[#CONST]]
+; CHECK:         OpAtomicStore %[[#]] %[[#WORKGROUP]] %[[#RELAXED]] %[[#CONST]]
+; CHECK:         OpAtomicLoad %[[#]] %[[#]] %[[#DEVICE]] %[[#SEQCST]]
+; CHECK:         OpAtomicLoad %[[#]] %[[#]] %[[#DEVICE]] %[[#RELAXED]]
+; CHECK:         OpAtomicLoad %[[#]] %[[#]] %[[#WORKGROUP]] %[[#RELAXED]]
 ; CHECK-COUNT-3: OpAtomicExchange
 
 define spir_kernel void @test_atomic_kernel(ptr addrspace(3) %ff) local_unnamed_addr #0 !kernel_arg_addr_space !3 !kernel_arg_access_qual !4 !kernel_arg_type !5 !kernel_arg_base_type !6 !kernel_arg_type_qual !7 {

jmmartinez · 2026-08-12T13:59:43Z

+  unsigned Semantics =
+      SPIRV::MemorySemantics::SequentiallyConsistent |
+      getMemSemanticsForStorageClass(GR->getPointerStorageClass(PtrRegister));
+  Register MemSemanticsReg =
+      Call->Arguments.size() >= 2 ? Call->Arguments[1] : Register();


It seems to me that buildMemSemanticsReg is fucked up, so we end up extracting the code of buildMemSemanticsReg to do what we want instead of fixing it.

Bear in mind the little context that I have, but I think that what we want is a buildMemSemanticsReg that takes an std::memory_order or a SPIRV::MemorySemantics (which would default to std::memory_order::memory_order_seq_cs/SPIRV::MemorySemantics::SequentiallyConsistent), and a pointer (to figure out its storage class). And returns a new register that points to a constant that represents this.

Currently the code may reuse the old register, so it fixes its storage class behind the scenes in an awkward way; and may modify the unsigned &Semantics that is passed.

Can we instead rewrite buildMemSemanticsReg such that we have a function that makes sense?

Done. Split it into computeMemSemantics and buildMemSemanticsReg

The code is still not clear.

From my point-of-view, the memory semantics are composed of 2 things:

a memory ordering scope that sometimes is derived from the pointer's storage class; and sometimes we seem to have forgotten about (e.g. in buildAtomicRMWInst, if the MemSemanticsReg is available we derive it from the pointer, if it is not, we use none);

a memory ordering this one is obtained from the register or we pick a per-operation default value.

Trying to jam these 2 things in the same function makes the code hard to follow (we end up pre-initializing the default value with the default with the default scope).

To try to make this easier to read, I'd split the handling of this and do something like:

// this unsigned DefaultSemantics = SPIRV::MemorySemantics::SequentiallyConsistent | getMemSemanticsForStorageClass(GR->getPointerStorageClass(PtrRegister)); Register MemSemanticsReg = Call->Arguments.size() >= 2 ? Call->Arguments[1] : Register(); MemSemanticsReg = buildMemSemanticsReg(MemSemanticsReg, PtrRegister, MIRBuilder, GR, DefaultSemantics);

// becomes SPIRV::MemorySemantics Ordering = SPIRV::MemorySemantics::SequentiallyConsistent; SPIRV::MemorySemantics OrderingScope = getMemSemanticsForStorageClass(GR->getPointerStorageClass(PtrRegister)); if(Call->Arguments.size() >= 2) { Ordering = getSPIRVMemSemantics(static_cast<std::memory_order>( getIConstVal(Call->Arguments[1], MIRBuilder.getMRI()))); } Register MemSemanticsReg = buildConstantIntReg32(Ordering | OrderingScope, MIRBuilder, GR);

In codes like buildAtomicRMWInst where the pointer ordering scope was ignored for the default value we can do:

// this Register MemSemanticsReg = Call->Arguments.size() >= 3 ? Call->Arguments[2] : Register(); MemSemanticsReg = buildMemSemanticsReg(MemSemanticsReg, PtrRegister, MIRBuilder, GR, SPIRV::MemorySemantics::None);

// becomes SPIRV::MemorySemantics Ordering = SPIRV::MemorySemantics::None; if(Call->Arguments.size() >= 3) { Ordering = getSPIRVMemSemantics(static_cast<std::memory_order>( getIConstVal(Call->Arguments[1], MIRBuilder.getMRI()))); } Register MemSemanticsReg = buildConstantIntReg32(Ordering, MIRBuilder, GR);

* bear in mind that this last behavior may be wrong. We may have to pick the ordering scope from the pointer and we simply forgot. Or maybe it doesn't apply for this instruction.

Thanks for the detailed reply!

Done, split further: getMemOrdering now only extracts the ordering from the operand, buildMemSemanticsReg just ORs it with the storage-class scope and materializes the constant

…d-store-explicit-args

aobolensk requested a review from jmmartinez August 11, 2026 14:04

llvmorg-github-actions Bot added the backend:SPIR-V label Aug 11, 2026

jmmartinez reviewed Aug 12, 2026

View reviewed changes

aobolensk added 4 commits August 13, 2026 08:42

Address comment

f2103dd

Merge branch 'main' into llvm-spirv-atomic-load-store-explicit-args

e86fbd2

Merge remote-tracking branch 'origin/main' into llvm-spirv-atomic-loa…

d3fa2f6

…d-store-explicit-args

Address comment

48852bb

Navigation Menu

Uh oh!

[SPIR-V] Honor explicit memory_order/memory_scope in atomic load/store - #215572

[SPIR-V] Honor explicit memory_order/memory_scope in atomic load/store#215572
aobolensk wants to merge 5 commits into
llvm:mainfrom
aobolensk:llvm-spirv-atomic-load-store-explicit-args

aobolensk commented Aug 11, 2026

Uh oh!

llvmorg-github-actions Bot commented Aug 11, 2026

Uh oh!

jmmartinez Aug 12, 2026

Uh oh!

aobolensk Aug 13, 2026

Uh oh!

jmmartinez Aug 14, 2026

Uh oh!

aobolensk Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aobolensk commented Aug 11, 2026

Uh oh!

llvmorg-github-actions Bot commented Aug 11, 2026

Uh oh!

jmmartinez Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

aobolensk Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

jmmartinez Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

aobolensk Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants