[AMDGPU] Save/Restore SCC bit across waterfall loop. #68363

srpande · 2023-10-05T22:51:45Z

Waterfall loop is overwriting SCC bit of status register. Make sure SCC bit is saved and restored across.
We need to save/restore only in cases where SCC is live across waterfall loop.

llvmbot · 2023-10-05T22:52:50Z

@llvm/pr-subscribers-backend-amdgpu

Changes

Waterfall loop is overwriting SCC bit of status register. Make sure SCC bit is saved and restored across.
We need to save/restore only in cases where SCC is live across waterfall loop.

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

3 Files Affected:

(modified) llvm/lib/Target/AMDGPU/SIInstrInfo.cpp (+50-1)
(modified) llvm/lib/Target/AMDGPU/SIInstrInfo.h (+13)
(added) llvm/test/CodeGen/AMDGPU/waterfall_kills_scc.ll (+87)

diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 792f4695d288b5f..cd9ad4fcb21c5d4 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -5094,6 +5094,39 @@ unsigned SIInstrInfo::getVALUOp(const MachineInstr &MI) const {
       "Unexpected scalar opcode without corresponding vector one!");
 }
 
+bool SIInstrInfo::isSCCDefinedBefore(MachineBasicBlock &MBB,
+                                     MachineBasicBlock::iterator Before) const {
+
+  for (MachineBasicBlock::iterator I = Before, B = MBB.begin(); I != B; --I) {
+    MachineInstr &MI = *I;
+    if (!MI.hasImplicitDef())
+      continue;
+    for (MachineOperand &Op : MI.implicit_operands()) {
+      if (Op.getReg() == AMDGPU::SCC && Op.isDef() && !Op.isDead())
+        return true;
+    }
+  }
+  return false;
+}
+
+bool SIInstrInfo::isSCCUsedAfter(MachineBasicBlock &MBB,
+                                 MachineBasicBlock::iterator After) const {
+  for (MachineBasicBlock::iterator I = After, E = MBB.end(); I != E; ++I) {
+    MachineInstr &MI = *I;
+    if (MI.hasRegisterImplicitUseOperand(AMDGPU::SCC))
+      return true;
+  }
+  return false;
+}
+
+bool SIInstrInfo::isSCCDefinedAndUsed(MachineBasicBlock &MBB,
+                                      MachineBasicBlock::iterator Before,
+                                      MachineBasicBlock::iterator After) const {
+  if (isSCCDefinedBefore(MBB, Before) && isSCCUsedAfter(MBB, After))
+    return true;
+  return false;
+}
+
 void SIInstrInfo::insertScratchExecCopy(MachineFunction &MF,
                                         MachineBasicBlock &MBB,
                                         MachineBasicBlock::iterator MBBI,
@@ -6014,6 +6047,16 @@ loadMBUFScalarOperandsFromVGPR(const SIInstrInfo &TII, MachineInstr &MI,
   unsigned MovExecOpc = ST.isWave32() ? AMDGPU::S_MOV_B32 : AMDGPU::S_MOV_B64;
   const auto *BoolXExecRC = TRI->getRegClass(AMDGPU::SReg_1_XEXECRegClassID);
 
+  // Save SCC. Waterfall Loop may overwrite SCC.
+  Register SaveSCCReg;
+  bool SCCDefined = false;
+  if ((SCCDefined = TII.isSCCDefinedAndUsed(MBB, Begin, End))) {
+    SaveSCCReg = MRI.createVirtualRegister(
+        TRI->getRegClass(AMDGPU::SReg_1_XEXECRegClassID));
+    BuildMI(MBB, Begin, DL, TII.get(AMDGPU::COPY), SaveSCCReg)
+        .addReg(AMDGPU::SCC);
+  }
+
   Register SaveExec = MRI.createVirtualRegister(BoolXExecRC);
 
   // Save the EXEC mask
@@ -6069,8 +6112,14 @@ loadMBUFScalarOperandsFromVGPR(const SIInstrInfo &TII, MachineInstr &MI,
 
   emitLoadScalarOpsFromVGPRLoop(TII, MRI, MBB, *LoopBB, *BodyBB, DL, ScalarOps);
 
-  // Restore the EXEC mask
   MachineBasicBlock::iterator First = RemainderBB->begin();
+  // Restore SCC
+  if (SCCDefined) {
+    BuildMI(*RemainderBB, First, DL, TII.get(AMDGPU::COPY), AMDGPU::SCC)
+        .addReg(SaveSCCReg);
+  }
+
+  // Restore the EXEC mask
   BuildMI(*RemainderBB, First, DL, TII.get(MovExecOpc), Exec).addReg(SaveExec);
   return BodyBB;
 }
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.h b/llvm/lib/Target/AMDGPU/SIInstrInfo.h
index a4f59fc3513d646..3a347017f1c85a1 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.h
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.h
@@ -960,6 +960,19 @@ class SIInstrInfo final : public AMDGPUGenInstrInfo {
 
   unsigned getVALUOp(const MachineInstr &MI) const;
 
+  /// Return true if SCC is deinfed and not dead
+  /// from Before to beginning of MBB
+  bool isSCCDefinedBefore(MachineBasicBlock &MBB,
+                          MachineBasicBlock::iterator Before) const;
+
+  /// Return true if SCC is used from After to end of MBB
+  bool isSCCUsedAfter(MachineBasicBlock &MBB,
+                      MachineBasicBlock::iterator After) const;
+
+  bool isSCCDefinedAndUsed(MachineBasicBlock &MBB,
+                           MachineBasicBlock::iterator Before,
+                           MachineBasicBlock::iterator After) const;
+
   void insertScratchExecCopy(MachineFunction &MF, MachineBasicBlock &MBB,
                              MachineBasicBlock::iterator MBBI,
                              const DebugLoc &DL, Register Reg, bool IsSCCLive,
diff --git a/llvm/test/CodeGen/AMDGPU/waterfall_kills_scc.ll b/llvm/test/CodeGen/AMDGPU/waterfall_kills_scc.ll
new file mode 100644
index 000000000000000..bdde0dd8c4a788a
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/waterfall_kills_scc.ll
@@ -0,0 +1,87 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 3
+; RUN:  llc -march=amdgcn -mcpu=gfx906 -verify-machineinstrs < %s | FileCheck -check-prefix=GFX906 %s
+declare float @llvm.amdgcn.raw.buffer.load.f32(<4 x i32>, i32, i32, i32 immarg) #0
+declare void @llvm.amdgcn.raw.buffer.store.f32(float, <4 x i32>, i32, i32, i32 immarg) #1
+
+; Check that the compiler doesn't crash with a "undefined physical register" error;
+; bb.0 sets SCC bit in s_cmp_eq_u32 s0, 1
+; bb.1 overrides it
+; bb.2 uses the value from bb.0
+; Preserve SCC across bb.1 with s_cselect_b64 s[6:7], -1, 0 -> s_and_b64 s[0:1], s[6:7], exec
+; Otherwise, we will see following error.
+;*** Bad machine code: Using an undefined physical register ***
+;- function:    foo
+;- basic block: %bb.3  (0x53198c0)
+;- instruction: %33.sub1:sgpr_128 = S_CSELECT_B32 1072693248, 0, implicit $scc
+;- operand 3:   implicit $scc
+
+
+define amdgpu_kernel void  @foo(i1 %cmp1) {
+; GFX906-LABEL: foo:
+; GFX906:       ; %bb.0: ; %entry
+; GFX906-NEXT:    s_mov_b32 s8, SCRATCH_RSRC_DWORD0
+; GFX906-NEXT:    s_mov_b32 s9, SCRATCH_RSRC_DWORD1
+; GFX906-NEXT:    s_mov_b32 s10, -1
+; GFX906-NEXT:    s_mov_b32 s11, 0xe00000
+; GFX906-NEXT:    s_add_u32 s8, s8, s3
+; GFX906-NEXT:    s_addc_u32 s9, s9, 0
+; GFX906-NEXT:    buffer_load_dword v3, off, s[8:11], 0
+; GFX906-NEXT:    buffer_load_dword v4, off, s[8:11], 0 offset:4
+; GFX906-NEXT:    buffer_load_dword v5, off, s[8:11], 0 offset:8
+; GFX906-NEXT:    buffer_load_dword v6, off, s[8:11], 0 offset:12
+; GFX906-NEXT:    s_load_dword s4, s[0:1], 0x24
+; GFX906-NEXT:    s_load_dwordx2 s[2:3], s[0:1], 0x1c
+; GFX906-NEXT:    s_waitcnt lgkmcnt(0)
+; GFX906-NEXT:    s_bitcmp1_b32 s4, 0
+; GFX906-NEXT:    s_mul_i32 s0, s2, s3
+; GFX906-NEXT:    v_mul_u32_u24_e32 v1, s3, v1
+; GFX906-NEXT:    v_mad_u32_u24 v0, s0, v0, v1
+; GFX906-NEXT:    v_add_lshl_u32 v2, v0, v2, 4
+; GFX906-NEXT:    v_mov_b32_e32 v0, 0
+; GFX906-NEXT:    s_mov_b32 s4, 0
+; GFX906-NEXT:    v_mov_b32_e32 v1, v0
+; GFX906-NEXT:    s_cselect_b64 s[6:7], -1, 0
+; GFX906-NEXT:    s_mov_b64 s[2:3], exec
+; GFX906-NEXT:    ds_write_b64 v2, v[0:1]
+; GFX906-NEXT:  .LBB0_1: ; =>This Inner Loop Header: Depth=1
+; GFX906-NEXT:    s_waitcnt vmcnt(3)
+; GFX906-NEXT:    v_readfirstlane_b32 s0, v3
+; GFX906-NEXT:    s_waitcnt vmcnt(2)
+; GFX906-NEXT:    v_readfirstlane_b32 s1, v4
+; GFX906-NEXT:    v_cmp_eq_u64_e32 vcc, s[0:1], v[3:4]
+; GFX906-NEXT:    s_waitcnt vmcnt(1)
+; GFX906-NEXT:    v_readfirstlane_b32 s0, v5
+; GFX906-NEXT:    s_waitcnt vmcnt(0)
+; GFX906-NEXT:    v_readfirstlane_b32 s1, v6
+; GFX906-NEXT:    v_cmp_eq_u64_e64 s[0:1], s[0:1], v[5:6]
+; GFX906-NEXT:    s_and_b64 s[0:1], vcc, s[0:1]
+; GFX906-NEXT:    s_and_saveexec_b64 s[0:1], s[0:1]
+; GFX906-NEXT:    ; implicit-def: $vgpr3_vgpr4_vgpr5_vgpr6
+; GFX906-NEXT:    s_xor_b64 exec, exec, s[0:1]
+; GFX906-NEXT:    s_cbranch_execnz .LBB0_1
+; GFX906-NEXT:  ; %bb.2:
+; GFX906-NEXT:    s_and_b64 s[0:1], s[6:7], exec
+; GFX906-NEXT:    s_mov_b64 exec, s[2:3]
+; GFX906-NEXT:    s_cselect_b32 s5, 0x3ff00000, 0
+; GFX906-NEXT:    v_cvt_f32_f64_e32 v0, s[4:5]
+; GFX906-NEXT:    s_mov_b32 s5, s4
+; GFX906-NEXT:    s_mov_b32 s6, s4
+; GFX906-NEXT:    s_mov_b32 s7, s4
+; GFX906-NEXT:    buffer_store_dword v0, off, s[4:7], 0
+; GFX906-NEXT:    s_endpgm
+entry:
+  %wbr = alloca <4 x i32>, align 16, addrspace(5)
+  store ptr null, ptr addrspace(5) %wbr, align 16
+  %wbr_1 = load <4 x i32>, ptr addrspace(5) null, align 16
+  %call1 = tail call float @llvm.amdgcn.raw.buffer.load.f32(<4 x i32> %wbr_1, i32 0, i32 0, i32 0)
+  %0 = fpext float %call1 to double
+  %sel1 = select i1 %cmp1, double 1.000000e+00, double 0.000000e+00
+  %sel2 = select i1 %cmp1, double %0, double 0.000000e+00
+  %mul = fmul double %sel2, 0.000000e+00
+  %fptruncate = fptrunc double %sel1 to float
+  tail call void @llvm.amdgcn.raw.buffer.store.f32(float %fptruncate, <4 x i32> zeroinitializer, i32 0, i32 0, i32 0)
+  ret void
+}
+
+attributes #0 = { nocallback nofree nosync nounwind willreturn memory(read) }
+attributes #1 = { nocallback nofree nosync nounwind willreturn memory(write) }

srpande · 2023-10-05T22:57:49Z

I have another patch that does not use 64-bit SGPR registers to save/restore SCC.

arsenm

Also should fix this in the generalized waterfall handling in AMDGPURegBankSelect for GlobalISel

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

jayfoad · 2023-10-10T09:00:32Z

I have no comments on this patch, but I do think we should plan to improve waterfall formation in the future in a way that will make this patch obsolete.

Fundamentally we should not be forming waterfall loops in moveToVALU. That decision should be made during instruction selection, where we have accurate divergence information for buffer descriptors. ISel could either emit different opcodes for "buffer load with/without waterfall loop", or it could emit some extra pseudos marking the start/end of a required waterfall loop. In either case the emitted instructions can accurately say whether they will clobber SCC, and they can be expanded to real control flow in EmitInstrWithCustomInserter.

jayfoad · 2023-10-10T09:01:33Z

Also should fix this in the generalized waterfall handling in AMDGPURegBankSelect for GlobalISel

Surely GlobalISel Just Works because it is emitting real instructions that clobber SCC, so the register allocator can see exactly what's going on?

srpande · 2023-10-12T15:18:05Z

I have no comments on this patch, but I do think we should plan to improve waterfall formation in the future in a way that will make this patch obsolete.

Fundamentally we should not be forming waterfall loops in moveToVALU. That decision should be made during instruction selection, where we have accurate divergence information for buffer descriptors. ISel could either emit different opcodes for "buffer load with/without waterfall loop", or it could emit some extra pseudos marking the start/end of a required waterfall loop. In either case the emitted instructions can accurately say whether they will clobber SCC, and they can be expanded to real control flow in EmitInstrWithCustomInserter.

I agree Waterfall should be generated in ISel. I will open a draft to reimplement Waterfalll loop in ISel.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

Waterfall loop is overwriting SCC bit of status register. Make sure SCC bit is saved and restored. We need to save/restore only in cases where SCC is live across waterfall loop.

perlfu · 2023-10-18T00:35:10Z

Generally LGTM - but wait for other reviews to have a look.

ronlieb · 2023-10-18T11:26:20Z

@jayfoad patch ok to land ?

jayfoad

LGTM, thanks.

jayfoad · 2023-10-18T12:07:27Z

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

+  Register SaveSCCReg;
+  bool SCCNotDead = (MBB.computeRegisterLiveness(TRI, AMDGPU::SCC, MI, 30) !=
+                     MachineBasicBlock::LQR_Dead);
+  if (SCCNotDead) {


Nits: call this SCCLive? Even better, rewrite so you don't need it:

Suggested change

Register SaveSCCReg;

bool SCCNotDead = (MBB.computeRegisterLiveness(TRI, AMDGPU::SCC, MI, 30) !=

MachineBasicBlock::LQR_Dead);

if (SCCNotDead) {

Register SaveSCCReg;

if (MBB.computeRegisterLiveness(TRI, AMDGPU::SCC, MI, 30) !=

MachineBasicBlock::LQR_Dead) {

Then below you can just test:

// Restore SCC. if (SaveSCCReg) ...

llvmbot added the backend:AMDGPU label Oct 5, 2023

srpande requested review from jayfoad and arsenm October 5, 2023 22:52

arsenm reviewed Oct 5, 2023

View reviewed changes

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp Outdated Show resolved Hide resolved

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp Outdated Show resolved Hide resolved

srpande requested a review from arsenm October 5, 2023 23:12

perlfu reviewed Oct 10, 2023

View reviewed changes

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp Outdated Show resolved Hide resolved

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp Outdated Show resolved Hide resolved

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp Outdated Show resolved Hide resolved

srpande mentioned this pull request Oct 11, 2023

[AMDGPU] Use 32-bit SGPR to save/restore of SCC #68367

Open

srpande force-pushed the sirish/swdev-421509_save_restore_scc_only branch from 7b9db61 to 5c5b8f0 Compare October 12, 2023 15:18

jayfoad reviewed Oct 12, 2023

View reviewed changes

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp Outdated Show resolved Hide resolved

srpande requested a review from perlfu October 12, 2023 15:27

srpande force-pushed the sirish/swdev-421509_save_restore_scc_only branch from 5c5b8f0 to dcbd69b Compare October 12, 2023 16:31

jayfoad reviewed Oct 16, 2023

View reviewed changes

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp Outdated Show resolved Hide resolved

jayfoad reviewed Oct 16, 2023

View reviewed changes

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp Outdated Show resolved Hide resolved

[AMDGPU] Save/Restore SCC bit across waterfall loop.

3a71ebd

Waterfall loop is overwriting SCC bit of status register. Make sure SCC bit is saved and restored. We need to save/restore only in cases where SCC is live across waterfall loop.

srpande force-pushed the sirish/swdev-421509_save_restore_scc_only branch from dcbd69b to 3a71ebd Compare October 17, 2023 20:47

srpande requested a review from jayfoad October 17, 2023 20:50

jayfoad approved these changes Oct 18, 2023

View reviewed changes

srpande merged commit 28e4f97 into llvm:main Oct 18, 2023
2 checks passed

srpande deleted the sirish/swdev-421509_save_restore_scc_only branch October 18, 2023 16:02

madhur13490 mentioned this pull request Oct 20, 2023

Revert commit ba8565fbcb975e2d067ce3ae5a7dbaae4953edd3 madhur13490/llvm-project#3

Closed

srpande restored the sirish/swdev-421509_save_restore_scc_only branch October 20, 2023 15:44

banach-space mentioned this pull request Oct 24, 2023

[mlir][vector] Add scalable vectors to tests for vector.contract #70039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMDGPU] Save/Restore SCC bit across waterfall loop. #68363

[AMDGPU] Save/Restore SCC bit across waterfall loop. #68363

srpande commented Oct 5, 2023

llvmbot commented Oct 5, 2023

srpande commented Oct 5, 2023

arsenm left a comment

jayfoad commented Oct 10, 2023

jayfoad commented Oct 10, 2023

srpande commented Oct 12, 2023

perlfu commented Oct 18, 2023

ronlieb commented Oct 18, 2023

jayfoad left a comment

jayfoad Oct 18, 2023

[AMDGPU] Save/Restore SCC bit across waterfall loop. #68363

[AMDGPU] Save/Restore SCC bit across waterfall loop. #68363

Conversation

srpande commented Oct 5, 2023

llvmbot commented Oct 5, 2023

srpande commented Oct 5, 2023

arsenm left a comment

Choose a reason for hiding this comment

jayfoad commented Oct 10, 2023

jayfoad commented Oct 10, 2023

srpande commented Oct 12, 2023

perlfu commented Oct 18, 2023

ronlieb commented Oct 18, 2023

jayfoad left a comment

Choose a reason for hiding this comment

jayfoad Oct 18, 2023

Choose a reason for hiding this comment