[AMDGPU] Merge consecutive wait_alu instruction #128916

mihajlovicana · 2025-02-26T17:22:49Z

No description provided.

github-actions · 2025-02-26T17:23:10Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-02-26T17:23:45Z

@llvm/pr-subscribers-backend-amdgpu

Author: Ana Mihajlovic (mihajlovicana)

Changes

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

2 Files Affected:

(modified) llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp (+22)
(added) llvm/test/CodeGen/AMDGPU/merge-consecutive-wait-alus.mir (+30)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp b/llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp
index 4df55eac5d76b..bb15d12ada650 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp
@@ -164,6 +164,21 @@ class AMDGPUWaitSGPRHazards {
       BuildMI(MBB, MI, MI->getDebugLoc(), TII->get(AMDGPU::DS_NOP));
   }
 
+  unsigned mergeMasks(unsigned Mask1, unsigned Mask2) {
+    unsigned Mask = Mask1 & Mask2;
+
+    Mask = AMDGPU::DepCtr::encodeFieldVmVsrc(
+        Mask, std::min(AMDGPU::DepCtr::decodeFieldVmVsrc(Mask1),
+                       AMDGPU::DepCtr::decodeFieldVmVsrc(Mask2)));
+    Mask = AMDGPU::DepCtr::encodeFieldVaSdst(
+        Mask, std::min(AMDGPU::DepCtr::decodeFieldVaSdst(Mask1),
+                       AMDGPU::DepCtr::decodeFieldVaSdst(Mask2)));
+    Mask = AMDGPU::DepCtr::encodeFieldVaVdst(
+        Mask, std::min(AMDGPU::DepCtr::decodeFieldVaVdst(Mask1),
+                       AMDGPU::DepCtr::decodeFieldVaVdst(Mask2)));
+    return Mask;
+  }
+
   bool runOnMachineBasicBlock(MachineBasicBlock &MBB, bool Emit) {
     enum { WA_VALU = 0x1, WA_SALU = 0x2, WA_VCC = 0x4 };
 
@@ -362,6 +377,13 @@ class AMDGPUWaitSGPRHazards {
           Mask = AMDGPU::DepCtr::encodeFieldVaSdst(Mask, 0);
         }
         if (Emit) {
+          if (MI != MI->getParent()->begin()) {
+            MachineInstr &PrevMI = *std::prev(MI);
+            if (PrevMI.getOpcode() == AMDGPU::S_WAITCNT_DEPCTR) {
+              Mask = mergeMasks(Mask, PrevMI.getOperand(0).getImm());
+              PrevMI.eraseFromParent();
+            }
+          }
           auto NewMI = BuildMI(MBB, MI, MI->getDebugLoc(),
                                TII->get(AMDGPU::S_WAITCNT_DEPCTR))
                            .addImm(Mask);
diff --git a/llvm/test/CodeGen/AMDGPU/merge-consecutive-wait-alus.mir b/llvm/test/CodeGen/AMDGPU/merge-consecutive-wait-alus.mir
new file mode 100644
index 0000000000000..0cd203e6a9bbb
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/merge-consecutive-wait-alus.mir
@@ -0,0 +1,30 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc -mtriple=amdgcn -mcpu=gfx1200 -verify-machineinstrs -run-pass amdgpu-wait-sgpr-hazards -o -  %s | FileCheck %s
+
+
+---
+name: merge_consecutive_wait_alus
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $vgpr0
+
+    ; CHECK-LABEL: name: merge_consecutive_wait_alus
+    ; CHECK: liveins: $vgpr0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: renamable $sgpr0 = V_CMP_NE_U32_e64 0, $vgpr0, implicit $exec, implicit-def $vcc_lo, implicit-def $vcc_lo
+    ; CHECK-NEXT: S_WAITCNT_DEPCTR 61946
+    ; CHECK-NEXT: renamable $vgpr0 = V_CNDMASK_B32_e64 0, -1, 0, killed $vgpr0, killed $sgpr0, implicit $exec, implicit-def $vcc_lo
+    renamable $sgpr0 = V_CMP_NE_U32_e64 0, $vgpr0, implicit $exec, implicit-def $vcc_lo, implicit-def $vcc
+    S_WAITCNT_DEPCTR 65530
+    renamable $vgpr0 = V_CNDMASK_B32_e64 0, -1, 0, killed $vgpr0, killed $sgpr0, implicit $exec, implicit-def $vcc
+...
+
+
+## NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+# CHECK: {{.*}}

shiltian · 2025-02-26T17:52:49Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

@@ -362,6 +377,13 @@ class AMDGPUWaitSGPRHazards {
          Mask = AMDGPU::DepCtr::encodeFieldVaSdst(Mask, 0);
        }
        if (Emit) {
+          if (MI != MI->getParent()->begin()) {
+            MachineInstr &PrevMI = *std::prev(MI);
+            if (PrevMI.getOpcode() == AMDGPU::S_WAITCNT_DEPCTR) {


do we also want to check s_wait_alu here? They are aliases.

do we also want to check s_wait_alu here? They are aliases.

I think in MIR there is only S_WAITCNT_DEPCTR

nhaehnle · 2025-02-26T21:32:11Z

llvm/test/CodeGen/AMDGPU/merge-consecutive-wait-alus.mir

+## NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+# CHECK: {{.*}}


Left-over from an earlier version, should be removed.

arsenm · 2025-02-27T01:32:25Z

llvm/test/CodeGen/AMDGPU/merge-consecutive-wait-alus.mir

+    S_WAITCNT_DEPCTR 65530
+    renamable $vgpr0 = V_CNDMASK_B32_e64 0, -1, 0, killed $vgpr0, killed $sgpr0, implicit $exec, implicit-def $vcc
+...
+


Test where it's the start of the block and end of the block

Also add a test that skips meta instructions

Test where it's the start of the block and end of the block

Do you mean same basic block or successor and predecessor ?

arsenm · 2025-02-27T01:33:12Z

llvm/test/CodeGen/AMDGPU/merge-consecutive-wait-alus.mir

+    S_WAITCNT_DEPCTR 65530
+    renamable $vgpr0 = V_CNDMASK_B32_e64 0, -1, 0, killed $vgpr0, killed $sgpr0, implicit $exec, implicit-def $vcc
+...
+


Also add a test that skips meta instructions

arsenm · 2025-02-27T01:33:24Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

@@ -362,6 +377,13 @@ class AMDGPUWaitSGPRHazards {
          Mask = AMDGPU::DepCtr::encodeFieldVaSdst(Mask, 0);
        }
        if (Emit) {
+          if (MI != MI->getParent()->begin()) {
+            MachineInstr &PrevMI = *std::prev(MI);


You need to skip over debug instructions to avoid them changing the result

perlfu · 2025-02-27T05:12:43Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

@@ -164,6 +164,21 @@ class AMDGPUWaitSGPRHazards {
      BuildMI(MBB, MI, MI->getDebugLoc(), TII->get(AMDGPU::DS_NOP));
  }

+  unsigned mergeMasks(unsigned Mask1, unsigned Mask2) {
+    unsigned Mask = Mask1 & Mask2;


Need a comment to document why an AND operation here is appropriate.
I can reason that it is safe; however, I think using this method is embedded an implicit assumption that SaSdst and VaVcc are single bits when these should be handled like other fields below (i.e. using encode and decode).
Specifically comment needs to explain why the AND is safe for any undefined/undocumented bits in the mask.
Alternatively this function should fail gracefully for bits it doesn't explicit handle.

jayfoad · 2025-02-28T10:26:47Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+#include "llvm-c/Core.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/ilist_iterator.h"
+#include "llvm/CodeGen/MachineBasicBlock.h"
+#include "llvm/CodeGen/MachineInstr.h"
+#include "llvm/IR/CFG.h"
+#include <iterator>


Are any of these actually required? The llvm-c include looks particularly suspicious.

jayfoad · 2025-02-28T10:32:38Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+    return Mask;
+  }
+
+  MachineInstr *getPreviousWaitAlu(MachineBasicBlock::instr_iterator &MI) {


Function seems badly named, since it does not return a wait_alu instruction. Could you use the standard prev_nodbg or do you really need to skip meta instructions too for some reason?

Function seems badly named, since it does not return a wait_alu instruction. Could you use the standard prev_nodbg or do you really need to skip meta instructions too for some reason?

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sched.barrier.ll <- test where meta instructions are there until the end

OK... but is there a compelling reason to combine wait_alus on either side of a sched_barrier? That feels a bit like moving wait_alus across a sched_barrier, and the whole point of sched_barrier is to prevent that, isn't it?

what about other meta instructions ?

what about other meta instructions ?
@nhaehnle what do you think ?

Let's just only skip debug instructions, so that debug instructions affect code generation as little as possible.

jayfoad · 2025-02-28T10:34:03Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+          if (PrevWaitAlu != nullptr &&
+              PrevWaitAlu->getOpcode() == AMDGPU::S_WAITCNT_DEPCTR) {
+            Mask = mergeMasks(Mask, PrevWaitAlu->getOperand(0).getImm());
+            PrevWaitAlu->eraseFromParent();


Nit: updating the mask of the existing instruction should be a bit cheaper than erasing it and creating a new instruction.

jayfoad · 2025-02-28T11:44:14Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+            PrevWaitAlu = getPreviousWaitAlu(MI);
+          } else {
+            auto Preds = MBB.predecessors();
+            if (MBB.pred_size() == 1) {


I guess here you are relying on the fact that if the predecessor block ends with s_wait_alu then it must fall through into MBB, i.e. it only has a single successor. But does the single-successor --> single predecessor case really occur often enough to be worth handling here?

I think it is rare but I still ended up including it just in case.

@nhaehnle I am also not sure about this one

As discussed offline, let's not look across different basic blocks.

jayfoad · 2025-03-06T14:43:33Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+            if (It->getOpcode() == AMDGPU::S_WAITCNT_DEPCTR) {
+              Mask = mergeMasks(Mask, It->getOperand(0).getImm());
+              It->getOperand(0).setImm(Mask);
+              continue;


This will skip the "Update hazards based on defs" code below. Is that really safe?

Also I think you still need to set Emitted = true in this path, since it will be used to determine whether this pass changed anything at all in the MIR.

Yes code below should not be skipped.

Thanks for pointing this out !

jayfoad

Looks much cleaner now, thanks.

jayfoad

LGTM, just some optional nits inline.

jayfoad · 2025-03-07T10:27:23Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+    return Mask;
+  }
+
+  bool mergeSubsequentWaitAlus(MachineBasicBlock::instr_iterator &MI,


Maybe:

Suggested change

bool mergeSubsequentWaitAlus(MachineBasicBlock::instr_iterator &MI,

bool mergeConsecutiveWaitAlus(MachineBasicBlock::instr_iterator &MI,

jayfoad · 2025-03-07T10:28:01Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+  bool mergeSubsequentWaitAlus(MachineBasicBlock::instr_iterator &MI,
+                               unsigned Mask) {
+    auto MBB = MI->getParent();
+    if (MI != MBB->instr_begin()) {


Could use early returns in this function to reduce indentation.

auto MBB = MI->getParent(); if (MI == MBB->instr_begin()) return false; MachineBasicBlock::instr_iterator It = std::prev(MI); while (It != MBB->instr_begin() && It->isDebugInstr()) --It; if (It->getOpcode() == AMDGPU::S_WAITCNT_DEPCTR) { It->getOperand(0).setImm(mergeMasks(Mask, It->getOperand(0).getImm())); return true; } return false; } ```

Do you mean something like this ?

Yes. You can go even further:

if (It->getOpcode() != AMDGPU::S_WAITCNT_DEPCTR) return false; It->getOperand(0).setImm(mergeMasks(Mask, It->getOperand(0).getImm())); return true;

It is a matter of taste, but it is pretty common to do this in LLVM code.

jayfoad · 2025-03-07T10:28:26Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+    auto MBB = MI->getParent();
+    if (MI != MBB->instr_begin()) {
+      MachineBasicBlock::instr_iterator It = std::prev(MI);
+      while (It != MBB->instr_begin() && It->isDebugInstr())


Could use getPrevNonDebugInstruction?

I tried searching for a function like that but I only found getLastNonDebugInstr() and getFirstNonDebugInstr() for a given Machine BB but not something that retrieves previous non debug instruction relative to current Machine Instruction.

skipDebugInstructionsForward and skipDebugInstructionsBackward

jayfoad · 2025-03-07T10:57:26Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+      while (It != MBB->instr_begin() && It->isDebugInstr())
+        --It;


Sorry, I meant prev_nodbg not getPrevNonDebugInstruction:

Suggested change

while (It != MBB->instr_begin() && It->isDebugInstr())

--It;

It = prev_nodbg(It, MBB->instr_begin());

nhaehnle · 2025-03-10T16:20:58Z

llvm/lib/Target/AMDGPU/AMDGPUWaitSGPRHazards.cpp

+
+    return false;


Spurious return false.

nhaehnle · 2025-03-10T16:23:18Z

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

+}
+
+unsigned encodeFieldVaSsrc(unsigned VaSsrc) {
+  return encodeFieldVaSsrc(0xfff, VaSsrc);


Should be 0xffff?

nhaehnle · 2025-03-10T16:23:24Z

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

+}
+
+unsigned encodeFieldHoldCnt(unsigned HoldCnt) {
+  return encodeFieldHoldCnt(0xfff, HoldCnt);


Same here: 0xffff?

yes, that should be fixed

mihajlovicana · 2025-03-11T13:59:05Z

PING

nhaehnle

Thanks, LGTM

llvmbot added the backend:AMDGPU label Feb 26, 2025

shiltian requested review from arsenm, perlfu and jrbyrnes February 26, 2025 17:50

shiltian reviewed Feb 26, 2025

View reviewed changes

nhaehnle reviewed Feb 26, 2025

View reviewed changes

arsenm reviewed Feb 27, 2025

View reviewed changes

perlfu reviewed Feb 27, 2025

View reviewed changes

jayfoad reviewed Feb 28, 2025

View reviewed changes

mihajlovicana added 2 commits March 6, 2025 15:24

merge consecutive wait_alu instructions

5752242

skip debug instructions

8d02e56

mihajlovicana force-pushed the merge_wait_alu branch from e1792b7 to 8d02e56 Compare March 6, 2025 14:34

jayfoad reviewed Mar 6, 2025

View reviewed changes

removed skipping hazard state update

f093ec6

jayfoad approved these changes Mar 7, 2025

View reviewed changes

jayfoad reviewed Mar 7, 2025

View reviewed changes

nhaehnle reviewed Mar 10, 2025

View reviewed changes

style changes, small fix

bba8686

mihajlovicana force-pushed the merge_wait_alu branch from 3c7007e to bba8686 Compare March 11, 2025 09:43

nhaehnle approved these changes Mar 11, 2025

View reviewed changes

		## NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
		# CHECK: {{.*}}

	bool mergeSubsequentWaitAlus(MachineBasicBlock::instr_iterator &MI,
	bool mergeConsecutiveWaitAlus(MachineBasicBlock::instr_iterator &MI,

	while (It != MBB->instr_begin() && It->isDebugInstr())
	--It;
	It = prev_nodbg(It, MBB->instr_begin());

[AMDGPU] Merge consecutive wait_alu instruction #128916

Are you sure you want to change the base?

[AMDGPU] Merge consecutive wait_alu instruction #128916

Conversation

mihajlovicana commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

llvmbot commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihajlovicana Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayfoad left a comment

Choose a reason for hiding this comment

jayfoad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihajlovicana commented Mar 11, 2025

nhaehnle left a comment

Choose a reason for hiding this comment

mihajlovicana Feb 28, 2025 •

edited

Loading