[AMDGPU][NFC] Refactor FLAT_Global_* pseudos. #120244

sstipano · 2024-12-17T15:30:13Z

No description provided.

llvmbot · 2024-12-17T15:30:52Z

@llvm/pr-subscribers-backend-amdgpu

Author: None (sstipano)

Changes

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

1 Files Affected:

(modified) llvm/lib/Target/AMDGPU/FLATInstructions.td (+53-54)

diff --git a/llvm/lib/Target/AMDGPU/FLATInstructions.td b/llvm/lib/Target/AMDGPU/FLATInstructions.td
index 8fa708b74dde32..b2d28b0262a9e9 100644
--- a/llvm/lib/Target/AMDGPU/FLATInstructions.td
+++ b/llvm/lib/Target/AMDGPU/FLATInstructions.td
@@ -205,13 +205,13 @@ class GlobalSaddrTable <bit is_saddr, string Name = ""> {
 class FLAT_Load_Pseudo <string opName, RegisterClass regClass,
   bit HasTiedOutput = 0,
   bit HasSaddr = 0, bit EnableSaddr = 0,
+  RegisterClass VaddrRC = !if(EnableSaddr, VGPR_32, VReg_64),
   RegisterOperand vdata_op = getLdStRegisterOperand<regClass>.ret> : FLAT_Pseudo<
   opName,
   (outs vdata_op:$vdst),
   !con(
     !if(EnableSaddr,
-      (ins SReg_64_XEXEC_XNULL:$saddr, VGPR_32:$vaddr),
-      (ins VReg_64:$vaddr)),
+      (ins SReg_64_XEXEC_XNULL:$saddr), (ins)), (ins VaddrRC:$vaddr),
       (ins flat_offset:$offset),
       // FIXME: Operands with default values do not work with following non-optional operands.
       !if(HasTiedOutput, (ins CPol:$cpol, vdata_op:$vdst_in),
@@ -227,14 +227,13 @@ class FLAT_Load_Pseudo <string opName, RegisterClass regClass,
 }
 
 class FLAT_Store_Pseudo <string opName, RegisterClass vdataClass,
-  bit HasSaddr = 0, bit EnableSaddr = 0> : FLAT_Pseudo<
+  bit HasSaddr = 0, bit EnableSaddr = 0,
+  RegisterClass VaddrRC = !if(EnableSaddr, VGPR_32, VReg_64),
+  RegisterOperand vdata_op = getLdStRegisterOperand<vdataClass>.ret> : FLAT_Pseudo<
   opName,
   (outs),
-  !con(
-    !if(EnableSaddr,
-      (ins VGPR_32:$vaddr, getLdStRegisterOperand<vdataClass>.ret:$vdata, SReg_64_XEXEC_XNULL:$saddr),
-      (ins VReg_64:$vaddr, getLdStRegisterOperand<vdataClass>.ret:$vdata)),
-      (ins flat_offset:$offset, CPol_0:$cpol)),
+  !con((ins VaddrRC:$vaddr, vdata_op:$vdata), !if(EnableSaddr, (ins SReg_64_XEXEC_XNULL:$saddr), (ins)),
+       (ins flat_offset:$offset, CPol_0:$cpol)),
   " $vaddr, $vdata"#!if(HasSaddr, !if(EnableSaddr, ", $saddr", ", off"), "")#"$offset$cpol"> {
   let mayLoad  = 0;
   let mayStore = 1;
@@ -569,65 +568,65 @@ multiclass FLAT_Atomic_Pseudo<
   defm "" : FLAT_Atomic_Pseudo_RTN<opName, vdst_rc, vt, data_vt, data_rc, data_op>;
 }
 
-multiclass FLAT_Global_Atomic_Pseudo_NO_RTN<
+class FLAT_Global_Atomic_Pseudo_NO_RTN<
   string opName,
   RegisterClass vdst_rc,
   ValueType vt,
   ValueType data_vt = vt,
   RegisterClass data_rc = vdst_rc,
-  RegisterOperand data_op = getLdStRegisterOperand<data_rc>.ret> {
-
-  let is_flat_global = 1 in {
-    def "" : FLAT_AtomicNoRet_Pseudo <opName,
+  bit EnableSaddr = 0,
+  RegisterClass VaddrRC = !if(EnableSaddr, VGPR_32, VReg_64),
+  RegisterOperand data_op = getLdStRegisterOperand<data_rc>.ret> :
+    FLAT_AtomicNoRet_Pseudo <opName,
       (outs),
-      (ins VReg_64:$vaddr, data_op:$vdata, flat_offset:$offset, CPol_0:$cpol),
-      " $vaddr, $vdata, off$offset$cpol">,
+      !con((ins VaddrRC:$vaddr, data_op:$vdata), !if(EnableSaddr, (ins SReg_64_XEXEC_XNULL:$saddr), (ins)),
+           (ins flat_offset:$offset, CPol_0:$cpol)),
+      " $vaddr, $vdata, "#!if(EnableSaddr, "$saddr", "off")#"$offset$cpol">,
       GlobalSaddrTable<0, opName> {
-      let has_saddr = 1;
-      let FPAtomic = data_vt.isFP;
-    }
+  let has_saddr = 1;
+  let enabled_saddr = EnableSaddr;
+  let FPAtomic = data_vt.isFP;
+  let is_flat_global = 1;
+}
 
-    def _SADDR : FLAT_AtomicNoRet_Pseudo <opName,
-      (outs),
-      (ins VGPR_32:$vaddr, data_op:$vdata, SReg_64_XEXEC_XNULL:$saddr, flat_offset:$offset, CPol_0:$cpol),
-      " $vaddr, $vdata, $saddr$offset$cpol">,
-      GlobalSaddrTable<1, opName> {
-      let has_saddr = 1;
-      let enabled_saddr = 1;
-      let FPAtomic = data_vt.isFP;
-    }
-  }
+multiclass FLAT_Global_Atomic_Pseudo_Helper_NO_RTN<string opName,
+  RegisterClass vdst_rc,
+  ValueType vt,
+  ValueType data_vt = vt,
+  RegisterClass data_rc = vdst_rc> {
+  def "" : FLAT_Global_Atomic_Pseudo_NO_RTN<opName, vdst_rc, vt, data_vt, data_rc, 0>;
+  def _SADDR : FLAT_Global_Atomic_Pseudo_NO_RTN<opName, vdst_rc, vt, data_vt, data_rc, 1>;
 }
 
-multiclass FLAT_Global_Atomic_Pseudo_RTN<
+class FLAT_Global_Atomic_Pseudo_RTN<
   string opName,
   RegisterClass vdst_rc,
   ValueType vt,
   ValueType data_vt = vt,
   RegisterClass data_rc = vdst_rc,
+  bit EnableSaddr = 0,
+  RegisterClass VaddrRC = !if(EnableSaddr, VGPR_32, VReg_64),
   RegisterOperand data_op = getLdStRegisterOperand<data_rc>.ret,
-  RegisterOperand vdst_op = getLdStRegisterOperand<vdst_rc>.ret> {
-
-  let is_flat_global = 1 in {
-    def _RTN : FLAT_AtomicRet_Pseudo <opName,
+  RegisterOperand vdst_op = getLdStRegisterOperand<vdst_rc>.ret> :
+    FLAT_AtomicRet_Pseudo <opName,
       (outs vdst_op:$vdst),
-        (ins VReg_64:$vaddr, data_op:$vdata, flat_offset:$offset, CPol_GLC1:$cpol),
-      " $vdst, $vaddr, $vdata, off$offset$cpol">,
+      !con((ins VaddrRC:$vaddr, data_op:$vdata), !if(EnableSaddr, (ins SReg_64_XEXEC_XNULL:$saddr), (ins)),
+           (ins flat_offset:$offset, CPol_GLC1:$cpol)),
+      " $vdst, $vaddr, $vdata, "#!if(EnableSaddr, "$saddr", "off")#"$offset$cpol">,
       GlobalSaddrTable<0, opName#"_rtn"> {
-      let has_saddr = 1;
-      let FPAtomic = data_vt.isFP;
-    }
+  let has_saddr = 1;
+  let enabled_saddr = EnableSaddr;
+  let FPAtomic = data_vt.isFP;
+  let is_flat_global = 1;
+}
 
-    def _SADDR_RTN : FLAT_AtomicRet_Pseudo <opName,
-      (outs vdst_op:$vdst),
-        (ins VGPR_32:$vaddr, data_op:$vdata, SReg_64_XEXEC_XNULL:$saddr, flat_offset:$offset, CPol_GLC1:$cpol),
-      " $vdst, $vaddr, $vdata, $saddr$offset$cpol">,
-      GlobalSaddrTable<1, opName#"_rtn"> {
-       let has_saddr = 1;
-       let enabled_saddr = 1;
-       let FPAtomic = data_vt.isFP;
-    }
-  }
+multiclass FLAT_Global_Atomic_Pseudo_Helper_RTN<string opName,
+  RegisterClass vdst_rc,
+  ValueType vt,
+  ValueType data_vt = vt,
+  RegisterClass data_rc = vdst_rc> {
+  def _RTN : FLAT_Global_Atomic_Pseudo_RTN<opName, vdst_rc, vt, data_vt, data_rc, 0>;
+  def _SADDR_RTN : FLAT_Global_Atomic_Pseudo_RTN<opName, vdst_rc, vt, data_vt, data_rc, 1>;
 }
 
 multiclass FLAT_Global_Atomic_Pseudo<
@@ -636,8 +635,8 @@ multiclass FLAT_Global_Atomic_Pseudo<
   ValueType vt,
   ValueType data_vt = vt,
   RegisterClass data_rc = vdst_rc> {
-  defm "" : FLAT_Global_Atomic_Pseudo_NO_RTN<opName, vdst_rc, vt, data_vt, data_rc>;
-  defm "" : FLAT_Global_Atomic_Pseudo_RTN<opName, vdst_rc, vt, data_vt, data_rc>;
+  defm "" : FLAT_Global_Atomic_Pseudo_Helper_NO_RTN<opName, vdst_rc, vt, data_vt, data_rc>;
+  defm "" : FLAT_Global_Atomic_Pseudo_Helper_RTN<opName, vdst_rc, vt, data_vt, data_rc>;
 }
 
 //===----------------------------------------------------------------------===//
@@ -1018,19 +1017,19 @@ let SubtargetPredicate = isGFX10Plus in {
 } // End SubtargetPredicate = isGFX10Plus
 
 let OtherPredicates = [HasAtomicFaddNoRtnInsts] in
-  defm GLOBAL_ATOMIC_ADD_F32 : FLAT_Global_Atomic_Pseudo_NO_RTN <
+  defm GLOBAL_ATOMIC_ADD_F32 : FLAT_Global_Atomic_Pseudo_Helper_NO_RTN <
     "global_atomic_add_f32", VGPR_32, f32
   >;
 let OtherPredicates = [HasAtomicBufferGlobalPkAddF16NoRtnInsts] in
-  defm GLOBAL_ATOMIC_PK_ADD_F16 : FLAT_Global_Atomic_Pseudo_NO_RTN <
+  defm GLOBAL_ATOMIC_PK_ADD_F16 : FLAT_Global_Atomic_Pseudo_Helper_NO_RTN <
     "global_atomic_pk_add_f16", VGPR_32, v2f16
   >;
 let OtherPredicates = [HasAtomicFaddRtnInsts] in
-  defm GLOBAL_ATOMIC_ADD_F32 : FLAT_Global_Atomic_Pseudo_RTN <
+  defm GLOBAL_ATOMIC_ADD_F32 : FLAT_Global_Atomic_Pseudo_Helper_RTN <
     "global_atomic_add_f32", VGPR_32, f32
   >;
 let OtherPredicates = [HasAtomicBufferGlobalPkAddF16Insts] in
-  defm GLOBAL_ATOMIC_PK_ADD_F16 : FLAT_Global_Atomic_Pseudo_RTN <
+  defm GLOBAL_ATOMIC_PK_ADD_F16 : FLAT_Global_Atomic_Pseudo_Helper_RTN <
     "global_atomic_pk_add_f16", VGPR_32, v2f16
   >;

llvm/lib/Target/AMDGPU/FLATInstructions.td

arsenm · 2025-09-12T08:45:12Z

llvm/lib/Target/AMDGPU/FLATInstructions.td

I think this is a formatting regression, the ins should be indented to the same level

arsenm · 2025-09-12T08:45:40Z

llvm/lib/Target/AMDGPU/FLATInstructions.td

Each case on its own line?

This is kind of a counter example of the previous comment. In this case, if we were to have ins indented to the same level, first ins' indentation would seem wrong to me.

arsenm · 2025-09-12T08:45:53Z

llvm/lib/Target/AMDGPU/FLATInstructions.td

Suggested change

bit EnableSaddr = 0,

bit IsVGPR = 0,

bit EnableSaddr = false,

bit IsVGPR = false,

Sisyph

Regardless of the indentation decisions, this looks nice for avoiding duplication and making extension easier.

Sisyph · 2025-09-12T19:15:11Z

llvm/lib/Target/AMDGPU/FLATInstructions.td

I see there might be multiple opinions on what the indentation represent. In my opinion, the level of indentation should represent the nesting hierarchy of the lists or dags. (ins VaddrRC:$vaddr, flat_offset:$offset) should be at the same alignment level as !if, because they are both arguments of !con, and so at the same level in the nesting hierarchy. that (ins VaddrRC...) dag is not inside the if, which this alignment would to me.

Sisyph · 2025-09-12T19:21:30Z

llvm/lib/Target/AMDGPU/FLATInstructions.td

I would put !if(EnableSadd... ) on a new line at the same indentation as the two (ins...). They are all arguments to !con.

Sisyph · 2025-09-12T19:27:32Z

llvm/lib/Target/AMDGPU/FLATInstructions.td

Indentation here looks perfect to me!

llvm-ci · 2025-09-15T15:20:53Z

LLVM Buildbot has detected a new failure on builder openmp-s390x-linux running on systemz-1 while building llvm at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/16092

Here is the relevant piece of the build log for the reference

Step 6 (test-openmp) failure: test (failure)
******************** TEST 'libomp :: tasking/issue-94260-2.c' FAILED ********************
Exit Code: -11

Command Output (stdout):
--
# RUN: at line 1
/home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp   -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic && /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11

--

********************

sstipano requested a review from jayfoad December 17, 2024 15:30

llvmbot added the backend:AMDGPU label Dec 17, 2024

jayfoad reviewed Dec 17, 2024

View reviewed changes

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

sstipano force-pushed the flat_refactor branch from 5d20672 to ecf9a3f Compare December 17, 2024 16:13

sstipano requested a review from jayfoad December 17, 2024 16:14

sstipano force-pushed the flat_refactor branch from ecf9a3f to fb85b1b Compare September 12, 2025 00:10

sstipano requested review from arsenm, Sisyph, rampitec and mariusz-sikora-at-amd September 12, 2025 00:11

sstipano force-pushed the flat_refactor branch from fb85b1b to 4ea9f35 Compare September 12, 2025 00:14

arsenm reviewed Sep 12, 2025

View reviewed changes

sstipano force-pushed the flat_refactor branch from 4ea9f35 to 59d0558 Compare September 12, 2025 16:37

Sisyph approved these changes Sep 12, 2025

View reviewed changes

[AMDGPU][NFC] Refactor FLAT_Global_* pseudos.

6239732

sstipano force-pushed the flat_refactor branch from 59d0558 to 6239732 Compare September 15, 2025 13:48

sstipano merged commit e299d9a into llvm:main Sep 15, 2025
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AMDGPU][NFC] Refactor FLAT_Global_* pseudos. #120244

[AMDGPU][NFC] Refactor FLAT_Global_* pseudos. #120244

Uh oh!

sstipano commented Dec 17, 2024

Uh oh!

llvmbot commented Dec 17, 2024

Uh oh!

Uh oh!

arsenm Sep 12, 2025

Uh oh!

arsenm Sep 12, 2025

Uh oh!

sstipano Sep 12, 2025

Uh oh!

arsenm Sep 12, 2025

Uh oh!

Sisyph left a comment

Uh oh!

Sisyph Sep 12, 2025

Uh oh!

Sisyph Sep 12, 2025

Uh oh!

Sisyph Sep 12, 2025

Uh oh!

Uh oh!

llvm-ci commented Sep 15, 2025

Uh oh!

Uh oh!

-  bit EnableSaddr = 0,
-  bit IsVGPR = 0,
+  bit EnableSaddr = false,
+  bit IsVGPR = false,

[AMDGPU][NFC] Refactor FLAT_Global_* pseudos. #120244

[AMDGPU][NFC] Refactor FLAT_Global_* pseudos. #120244

Uh oh!

Conversation

sstipano commented Dec 17, 2024

Uh oh!

llvmbot commented Dec 17, 2024

Uh oh!

Uh oh!

arsenm Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

sstipano Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Sisyph left a comment

Choose a reason for hiding this comment

Uh oh!

Sisyph Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Sisyph Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Sisyph Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Sep 15, 2025

Uh oh!

Uh oh!