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

[Sebastian Neubauer via Phabricator via llvm-commits]

sebastian-ne added inline comments.


================
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);
----------------
arsenm wrote:
> 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?
That sounds better, thanks. For stores, the first instruction is a `buffer_store VGPR, …`, so there is no previous instruction I can add a define to.
For a pseudo, what would be the best pass to remove it again? (Maybe SIInsertWaitcntsPass?)


================
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);
----------------
arsenm wrote:
>  Do you mean identity copies?
No, that is about the superfluous s_mov instructions that @critson noticed. I fixed that in a patch on top of this one (I’ll put that on Phabricator after removing the inline assembly).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96336