[AMDGPU] Add verifier for flat_scr_base_hi read hazard

[rampitec]

No description provided.

[rampitec]

• [AMDGPU] Add verifier for flat_scr_base_hi read hazard #170550 👈
• main

This stack of pull requests is managed by sgh.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Stanislav Mekhanoshin (rampitec)

Changes

Also excludes S_MOV_B64 from the prohibited instructions as
this is not required by the original ticket. We cannot fold
it though anyway because of the RC mismatch, but we have some
mir tests with such use.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/SIInstrInfo.cpp (+14-1)
• (added) llvm/test/MachineVerifier/AMDGPU/hazard-gfx1250-flat-src-hi.mir (+66)

diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 7535407741f1f..3943560078ce2 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -5808,6 +5808,18 @@ bool SIInstrInfo::verifyInstruction(const MachineInstr &MI,
     }
   }
 
+  if (ST.hasFlatScratchHiInB64InstHazard() && isSALU(MI) &&
+      MI.readsRegister(AMDGPU::SRC_FLAT_SCRATCH_BASE_HI, &RI) &&
+      Opcode != AMDGPU::S_MOV_B64) {
+    const MachineOperand *Dst = getNamedOperand(MI, AMDGPU::OpName::sdst);
+    if ((Dst && AMDGPU::getRegBitWidth(
+                    *RI.getRegClassForReg(MRI, Dst->getReg())) == 64) ||
+        Opcode == AMDGPU::S_BITCMP0_B64 || Opcode == AMDGPU::S_BITCMP1_B64) {
+      ErrInfo = "Instruction cannot read flat_scratch_base_hi";
+      return false;
+    }
+  }
+
   return true;
 }
 
@@ -6259,7 +6271,8 @@ bool SIInstrInfo::isLegalRegOperand(const MachineInstr &MI, unsigned OpIdx,
     return false;
 
   if (ST.hasFlatScratchHiInB64InstHazard() &&
-      MO.getReg() == AMDGPU::SRC_FLAT_SCRATCH_BASE_HI && isSALU(MI)) {
+      MO.getReg() == AMDGPU::SRC_FLAT_SCRATCH_BASE_HI && isSALU(MI) &&
+      Opc != AMDGPU::S_MOV_B64) {
     if (const MachineOperand *Dst = getNamedOperand(MI, AMDGPU::OpName::sdst)) {
       if (AMDGPU::getRegBitWidth(*RI.getRegClassForReg(MRI, Dst->getReg())) ==
           64)
diff --git a/llvm/test/MachineVerifier/AMDGPU/hazard-gfx1250-flat-src-hi.mir b/llvm/test/MachineVerifier/AMDGPU/hazard-gfx1250-flat-src-hi.mir
new file mode 100644
index 0000000000000..3b9b40c52d1ed
--- /dev/null
+++ b/llvm/test/MachineVerifier/AMDGPU/hazard-gfx1250-flat-src-hi.mir
@@ -0,0 +1,66 @@
+# RUN: not --crash llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx1250 -verify-machineinstrs -run-pass=none -filetype=null %s 2>&1 | FileCheck %s
+
+---
+name:            salu_64_bit_inst_reads_flat_scratch_base_hi
+tracksRegLiveness: true
+body:             |
+  bb.0:
+
+    %0:sreg_64 = IMPLICIT_DEF
+    $sgpr0_sgpr1 = IMPLICIT_DEF
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_ASHR_I64
+
+    %1:sreg_64 = S_ASHR_I64 %0:sreg_64, $src_flat_scratch_base_hi, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_LSHL_B64
+
+    %2:sreg_64 = S_LSHL_B64 %0:sreg_64, $src_flat_scratch_base_hi, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_LSHR_B64
+
+    %3:sreg_64 = S_LSHR_B64 %0:sreg_64, $src_flat_scratch_base_hi, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_BFE_I64
+
+    %4:sreg_64 = S_BFE_I64 %0:sreg_64, $src_flat_scratch_base_hi, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_BFE_U64
+
+    %5:sreg_64 = S_BFE_U64 %0:sreg_64, $src_flat_scratch_base_hi, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_BFM_B64
+
+    %6:sreg_64 = S_BFM_B64 $src_flat_scratch_base_hi, 1, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_BITCMP0_B64
+
+    S_BITCMP0_B64 %0:sreg_64, $src_flat_scratch_base_hi, implicit $scc, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_BITCMP1_B64
+
+    S_BITCMP1_B64 %0:sreg_64, $src_flat_scratch_base_hi, implicit $scc, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_BITREPLICATE_B64_B32
+
+    %7:sreg_64 = S_BITREPLICATE_B64_B32 $src_flat_scratch_base_hi, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_BITSET0_B64
+
+    $sgpr0_sgpr1 = S_BITSET0_B64 $src_flat_scratch_base_hi, $sgpr0_sgpr1, implicit-def $scc
+
+    ; CHECK: *** Bad machine code: Instruction cannot read flat_scratch_base_hi ***
+    ; CHECK: S_BITSET1_B64
+
+    $sgpr0_sgpr1 = S_BITSET1_B64 $src_flat_scratch_base_hi, $sgpr0_sgpr1, implicit-def $scc
+...

[jayfoad]

No real objection but... shouldn't verifyInstruction call isOperandLegal on all operands so that we don't have to duplicate checks like this?

[rampitec]

No real objection but... shouldn't verifyInstruction call isOperandLegal on all operands so that we don't have to duplicate checks like this?

I was surprised it does not, but it is a way bigger change.

[rampitec]

nullptr here is intentional, I do not mean to include 64-bit SRC_FLAT_SCRATCH_BASE.

[arsenm]

Why are these opcodes special cased?

[rampitec]

These do not have sdst but listed as hazard in the ticket.

[arsenm]

No real objection but... shouldn't verifyInstruction call isOperandLegal on all operands so that we don't have to duplicate checks like this?

I thought it did already