[llvm] [AMDGPU][CodeGen] Place `S_NOP` after `S_SETREG_IMM32_B32` in predecessor MBB (PR #209620)

[Son Tuan Vu via llvm-commits]

================
@@ -432,23 +433,68 @@ AMDGPULowerVGPREncoding::handleCoissue(MachineBasicBlock::instr_iterator I) {
   return I;
 }
 
+/// Returns whether \p MI is a S_SETREG_IMM32_B32(MODE).
+static bool isSetregMode(const MachineInstr &MI, const SIInstrInfo &TII) {
+  if (MI.getOpcode() != AMDGPU::S_SETREG_IMM32_B32)
+    return false;
+
+  const MachineOperand *SIMM16Op =
+      TII.getNamedOperand(MI, AMDGPU::OpName::simm16);
+  auto [HwRegId, _Offset, _Size] =
+      AMDGPU::Hwreg::HwregEncoding::decode(SIMM16Op->getImm());
+  return HwRegId == AMDGPU::Hwreg::ID_MODE;
+}
+
+/// Backtracks \p I in \p MBB until we hit a non-meta instruction and returns
+/// whether that instruction is a S_SETREG_IMM32_B32(MODE). Returns false when
+/// there are no non-meta instruction in [MBB.instr_begin(), It).
+static bool previousInstrIsSetRegMode(MachineBasicBlock::instr_iterator &It,
+                                      const MachineBasicBlock &MBB,
+                                      const SIInstrInfo &TII) {
+  while (It != MBB.begin()) {
+    It = std::prev(It);
+    if (isSetregMode(*It, TII))
+      return true;
+    if (!It->isMetaInstruction())
+      return false;
+  }
+  return false;
+}
+
 bool AMDGPULowerVGPREncoding::needNopBeforeSetVGPRMSB(
     MachineBasicBlock::instr_iterator I) {
-  while (I != MBB->begin()) {
-    I = std::prev(I);
-    if (I->getOpcode() == AMDGPU::S_SETREG_IMM32_B32) {
-      MachineOperand *SIMM16Op =
-          TII->getNamedOperand(*I, AMDGPU::OpName::simm16);
-      auto [HwRegId, Offset, Size] =
-          AMDGPU::Hwreg::HwregEncoding::decode(SIMM16Op->getImm());
-      if (HwRegId == AMDGPU::Hwreg::ID_MODE)
+  if (previousInstrIsSetRegMode(I, *MBB, *TII))
+    return true;
+  if (I != MBB->begin())
+    return false;
----------------
tyb0807 wrote:

I think there's a bug here. What happens when the instruction just before the insertion point is a real instruction and it happens to be the block's first instruction? `previousInstrIsSetRegMode` returns false but leaves `I == MBB->begin()`. The `if` at line 468 will not fire, and we'll do predecessor scan incorrectly. If that predecessor ends with `S_SETREG_IMM32_B32`, we emit a `NOP` that isn't needed.

https://github.com/llvm/llvm-project/pull/209620