AMDGPU: Add a subtarget feature for fine-grained remote memory support #96442

arsenm · 2024-06-23T20:18:15Z

Atomic access to fine-grained remote memory does not work on all
subtargets. Add a feature for targets where this is expected to work.

arsenm · 2024-06-23T20:18:25Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @arsenm and the rest of your teammates on Graphite

llvmbot · 2024-06-23T20:18:55Z

@llvm/pr-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

Changes

Atomic access to fine-grained remote memory does not work on all
subtargets. Add a feature for targets where this is expected to work.

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

2 Files Affected:

(modified) llvm/lib/Target/AMDGPU/AMDGPU.td (+12-2)
(modified) llvm/lib/Target/AMDGPU/GCNSubtarget.h (+8)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPU.td b/llvm/lib/Target/AMDGPU/AMDGPU.td
index 5bc0fe8bba608..7ff861f5b144d 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPU.td
+++ b/llvm/lib/Target/AMDGPU/AMDGPU.td
@@ -788,6 +788,14 @@ def FeatureFlatAtomicFaddF32Inst
   "Has flat_atomic_add_f32 instruction"
 >;
 
+def FeatureAgentScopeFineGrainedRemoteMemoryAtomics
+  : SubtargetFeature<"agent-scope-fine-grained-remote-memory-atomics",
+  "HasAgentScopeFineGrainedRemoteMemoryAtomics",
+  "true",
+  "Agent (device) scoped atomic operations not directly supported by "
+  "PCIe work for allocations in host or peer PCIe device memory"
+>;
+
 def FeatureDefaultComponentZero : SubtargetFeature<"default-component-zero",
   "HasDefaultComponentZero",
   "true",
@@ -1207,7 +1215,8 @@ def FeatureGFX12 : GCNSubtargetFeatureGeneration<"GFX12",
    FeatureUnalignedBufferAccess, FeatureUnalignedDSAccess,
    FeatureTrue16BitInsts, FeatureDefaultComponentBroadcast,
    FeatureMaxHardClauseLength32,
-   FeatureAtomicFMinFMaxF32GlobalInsts, FeatureAtomicFMinFMaxF32FlatInsts
+   FeatureAtomicFMinFMaxF32GlobalInsts, FeatureAtomicFMinFMaxF32FlatInsts,
+   FeatureAgentScopeFineGrainedRemoteMemoryAtomics
   ]
 >;
 
@@ -1415,7 +1424,8 @@ def FeatureISAVersion9_4_Common : FeatureSet<
    FeatureBackOffBarrier,
    FeatureKernargPreload,
    FeatureAtomicFMinFMaxF64GlobalInsts,
-   FeatureAtomicFMinFMaxF64FlatInsts
+   FeatureAtomicFMinFMaxF64FlatInsts,
+   FeatureAgentScopeFineGrainedRemoteMemoryAtomics
    ]>;
 
 def FeatureISAVersion9_4_0 : FeatureSet<
diff --git a/llvm/lib/Target/AMDGPU/GCNSubtarget.h b/llvm/lib/Target/AMDGPU/GCNSubtarget.h
index 966708db4f37c..c40efbdcf7f0b 100644
--- a/llvm/lib/Target/AMDGPU/GCNSubtarget.h
+++ b/llvm/lib/Target/AMDGPU/GCNSubtarget.h
@@ -174,6 +174,7 @@ class GCNSubtarget final : public AMDGPUGenSubtargetInfo,
   bool HasAtomicBufferPkAddBF16Inst = false;
   bool HasFlatAtomicFaddF32Inst = false;
   bool HasDefaultComponentZero = false;
+  bool HasAgentScopeFineGrainedRemoteMemoryAtomics = false;
   bool HasDefaultComponentBroadcast = false;
   /// The maximum number of instructions that may be placed within an S_CLAUSE,
   /// which is one greater than the maximum argument to S_CLAUSE. A value of 0
@@ -871,6 +872,13 @@ class GCNSubtarget final : public AMDGPUGenSubtargetInfo,
 
   bool hasFlatAtomicFaddF32Inst() const { return HasFlatAtomicFaddF32Inst; }
 
+  /// \return true if atomic operations targeting fine-grained memory work
+  /// correctly at device scope, in allocations in host or peer PCIe device
+  /// memory.
+  bool supportsAgentScopeFineGrainedRemoteMemoryAtomics() const {
+    return HasAgentScopeFineGrainedRemoteMemoryAtomics;
+  }
+
   bool hasDefaultComponentZero() const { return HasDefaultComponentZero; }
 
   bool hasDefaultComponentBroadcast() const {

AlexVlx · 2024-06-23T20:24:30Z

Hmm, I might need to reparse this but I'm not entirely sure how this is expected to work in general since PCI-E attachment is not always / entirely knowable at compile time. Have I missed some preamble discussion here?

arsenm · 2024-06-23T20:30:53Z

Hmm, I might need to reparse this but I'm not entirely sure how this is expected to work in general since PCI-E attachment is not always / entirely knowable at compile time. Have I missed some preamble discussion here?

We do statically know for some of the targets (mostly gfx12 and gfx940) that it's supposed to work. This is the "scope downgrade" vs. "nop" cases in the atomic support table

rampitec · 2024-06-24T09:42:50Z

We do statically know for some of the targets (mostly gfx12 and gfx940) that it's supposed to work. This is the "scope downgrade" vs. "nop" cases in the atomic support table

Actually not, we do not know the bus. Moreover, we know this is opposite.

arsenm · 2024-06-24T10:02:54Z

Actually not, we do not know the bus. Moreover, we know this is opposite.

On gfx940/gfx12, we don't need to know the bus to handle the agent scope case correctly. The instructions still function, just always at device scope. We do need to know the bus and/or location to handle the system scope correctly, which will come from the new metadata.

yxsamliu · 2024-06-24T15:44:10Z

If a sub target does not have this feature, does none of the atomic instructions work for fine-grained remote memory, including integer atomic add/xchg/cmpxchg?

arsenm · 2024-06-24T16:33:57Z

No, the string description mentions this is for the non-PCIe supported cases

yxsamliu · 2024-06-24T16:39:19Z

need some tests

arsenm · 2024-06-24T17:42:39Z

need some tests

This does nothing as it is. The real use patches have thousands of tests

llvm/lib/Target/AMDGPU/AMDGPU.td

arsenm

ping

github-actions · 2024-07-02T17:04:48Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jayfoad · 2024-07-04T09:28:53Z

llvm/include/llvm/CodeGen/MachineBranchProbabilityInfo.h

 #include "llvm/Pass.h"
 #include "llvm/Support/BranchProbability.h"

 namespace llvm {

-class MachineBranchProbabilityInfo {
+class MachineBranchProbabilityInfo : public ImmutablePass {


Why does the PR include all this unrelated stuff? Which part am I supposed to review? Normally I just look at the "Files changed" tab.

The parts I actually committed. I don't know why "martinboehme and others added 14 commits 12 hours ago" happened, adding all this other stuff to my personal branch. This has happened several other times, but not consistently

This isn't happening for the other patches further up the stack, just this one for some reason

I don't know how it ended up like this but I've gotten rid of these commits

jayfoad · 2024-07-04T10:43:07Z

llvm/lib/Target/AMDGPU/AMDGPU.td

@@ -788,6 +788,16 @@ def FeatureFlatAtomicFaddF32Inst
  "Has flat_atomic_add_f32 instruction"
 >;

+def FeatureAgentScopeFineGrainedRemoteMemoryAtomics


I'd prefer using "Device" (hardware terminology) instead of "Agent" (terminology from some compute API??) in the hardware feature name. Although I see "agent" is already pervasive in AMDGPUUsage so perhaps that ship has sailed.

I originally went with device and changed it to agent to match the syncscope spelling. This was the HSA name

arsenm · 2024-07-10T11:26:51Z

ping

arsenm · 2024-07-10T12:37:35Z

Merge activity

Jul 10, 8:37 AM EDT: @arsenm started a stack merge that includes this pull request via Graphite.
Jul 10, 8:41 AM EDT: Graphite rebased this pull request as part of a merge.
Jul 10, 8:43 AM EDT: @arsenm merged this pull request with Graphite.

Atomic access to fine-grained remote memory does not work on all subtargets. Add a feature for targets where this is expected to work.

llvm#96442) Atomic access to fine-grained remote memory does not work on all subtargets. Add a feature for targets where this is expected to work.

arsenm mentioned this pull request Jun 23, 2024

AMDGPU: Legalize v2f16 atomicrmw fadd for buffer fat pointers #95929

Merged

This was referenced Jun 23, 2024

AMDGPU: Handle legal v2bf16 atomicrmw fadd for gfx12 #95930

Merged

AMDGPU: Add subtarget feature for global atomic fadd denormal support #96443

Merged

AMDGPU: Add subtarget feature for memory atomic fadd f64 #96444

Merged

arsenm added the backend:AMDGPU label Jun 23, 2024 — with Graphite App

arsenm requested review from AlexVlx, jayfoad, mariusz-sikora-at-amd, Pierre-vh, rampitec, Sisyph, t-tye and yxsamliu June 23, 2024 20:19

arsenm marked this pull request as ready for review June 23, 2024 20:19

yxsamliu reviewed Jun 24, 2024

View reviewed changes

llvm/lib/Target/AMDGPU/AMDGPU.td Outdated Show resolved Hide resolved

arsenm force-pushed the users/arsenm/amdgpu-add-subtarget-feature-fine-grained-remote-memory-atomics branch from 2da0565 to 1a441c0 Compare June 25, 2024 09:10

Base automatically changed from users/arsenm/amdgpu-atomicrmw-fadd-buffer-v2bf16 to main June 25, 2024 15:45

arsenm force-pushed the users/arsenm/amdgpu-add-subtarget-feature-fine-grained-remote-memory-atomics branch from 1a441c0 to 302a99a Compare June 25, 2024 22:32

This was referenced Jun 26, 2024

AMDGPU: Handle remote/fine-grained memory in atomicrmw fmin/fmax lowering #96759

Merged

AMDGPU: Handle new atomicrmw metadata for fadd case #96760

Merged

arsenm force-pushed the users/arsenm/amdgpu-add-subtarget-feature-fine-grained-remote-memory-atomics branch 2 times, most recently from 438d5bb to 53a120c Compare June 28, 2024 12:40

This was referenced Jun 28, 2024

AMDGPU/GlobalISel: Legalize atomicrmw fmin/fmax #97048

Merged

AMDGPU: Remove flat/global atomic fadd v2bf16 intrinsics #97050

Merged

AMDGPU: Remove global/flat atomic fadd intrinics #97051

Merged

arsenm commented Jul 1, 2024

View reviewed changes

arsenm force-pushed the users/arsenm/amdgpu-add-subtarget-feature-fine-grained-remote-memory-atomics branch from 53a120c to 8a87e14 Compare July 2, 2024 17:01

arsenm force-pushed the users/arsenm/amdgpu-add-subtarget-feature-fine-grained-remote-memory-atomics branch 2 times, most recently from e0ae621 to 1a6ff86 Compare July 3, 2024 21:41

jayfoad reviewed Jul 4, 2024

View reviewed changes

arsenm force-pushed the users/arsenm/amdgpu-add-subtarget-feature-fine-grained-remote-memory-atomics branch 2 times, most recently from a060a2a to 76190f2 Compare July 4, 2024 09:42

jayfoad reviewed Jul 4, 2024

View reviewed changes

yxsamliu approved these changes Jul 10, 2024

View reviewed changes

AMDGPU: Add a subtarget feature for fine-grained remote memory support

2b6a7bb

Atomic access to fine-grained remote memory does not work on all subtargets. Add a feature for targets where this is expected to work.

arsenm force-pushed the users/arsenm/amdgpu-add-subtarget-feature-fine-grained-remote-memory-atomics branch from 76190f2 to 2b6a7bb Compare July 10, 2024 12:40

arsenm merged commit 78dcd02 into main Jul 10, 2024
4 of 6 checks passed

arsenm deleted the users/arsenm/amdgpu-add-subtarget-feature-fine-grained-remote-memory-atomics branch July 10, 2024 12:43

This was referenced Aug 2, 2024

IR/AMDGPU: Autoupgrade amdgpu-unsafe-fp-atomics attribute #101698

Merged

AMDGPU: Stop handling legacy amdgpu-unsafe-fp-atomics attribute #101699

Merged

arsenm mentioned this pull request Aug 22, 2024

AMDGPU: Remove flat/global fmin/fmax intrinsics #105642

Merged

AMDGPU: Add a subtarget feature for fine-grained remote memory support #96442

AMDGPU: Add a subtarget feature for fine-grained remote memory support #96442

Uh oh!

Conversation

arsenm commented Jun 23, 2024

Uh oh!

arsenm commented Jun 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 23, 2024

Uh oh!

AlexVlx commented Jun 23, 2024

Uh oh!

arsenm commented Jun 23, 2024

Uh oh!

rampitec commented Jun 24, 2024

Uh oh!

arsenm commented Jun 24, 2024

Uh oh!

yxsamliu commented Jun 24, 2024

Uh oh!

arsenm commented Jun 24, 2024

Uh oh!

yxsamliu commented Jun 24, 2024

Uh oh!

arsenm commented Jun 24, 2024

Uh oh!

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayfoad Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

jayfoad Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm commented Jul 10, 2024

Uh oh!

arsenm commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Uh oh!

arsenm commented Jun 23, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading

arsenm commented Jul 10, 2024 •

edited

Loading