[PATCH] D96336: [AMDGPU] Save VGPR of whole wave when spilling

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1029
+/// then flip EXEC (EXEC = EXEC ^ -1), then save the rest of the lanes and flip
+/// EXEC again to restore its original value.
+void SIRegisterInfo::buildWaveVGPRSpillLoadStore(MachineBasicBlock::iterator MI,
----------------
Can you spell out the expected instruction sequence in the comment


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1065-1071
+    // FIXME LLVM may not know that VGPR is live in other lanes, we need to mark
+    // it as live, otherwise the MachineIR verifier complains.
+    // Only add this if VGPR is currently not live.
+    BuildMI(*MBB, MI, DL, TII->get(AMDGPU::INLINEASM))
+        .addExternalSymbol("")
+        .addImm(0) // flags
+        .addReg(VGPR, RegState::ImplicitDefine);
----------------
Definitely should not be introducing inline asm. Can't you just add an implicit def on the first instruction in the sequence, or introduce a special purpose pseudo?


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1087
+    // Restore exec
+    // FIXME This often creates unnecessary exec moves
+    BuildMI(*MBB, MI, DL, TII->get(ExecMovOpc), ExecReg).addReg(SavedExecReg);
----------------
 Do you mean identity copies?


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1111
+        .addImm(InlineAsm::Extra_HasSideEffects) // flags
+        .addReg(VGPR, RegState::Implicit);
+  }
----------------
Can't you just add this as an implicit use to the last instruction in the sequence?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96336/new/

https://reviews.llvm.org/D96336