[PATCH] D124196: [AMDGPU][SILowerSGPRSpills] Spill SGPRs to virtual VGPRs

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:291-292
 
+    auto UpdateLaneVGPRDomInstr = [&](int FI, MachineBasicBlock *MBB,
+                                      MachineBasicBlock::iterator InsertPt) {
+      // For the Def of a virtual LaneVPGR to dominate all its uses, we should
----------------
arsenm wrote:
> Seems worthwhile for this to be its own real function instead of a lambda
Yes indeed. Will do.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:366
+          BuildMI(*InsertPt->getParent(), *InsertPt, InsertPt->getDebugLoc(),
+                  TII->get(AMDGPU::IMPLICIT_DEF), Reg);
+      if (LIS) {
----------------
arsenm wrote:
> It might be worth adding a target comment flag for this implicit def to comment it's for SGPR spilling
I don't think we can do any special handling for them at assembly emission.
IMPLICIT_DEF is handled/printed by the generic part of AsmPrinter and it won't reach the target-specific emitInstruction at all.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:394-395
+    // Virtual registers will get introduced and mostly with multiple defs.
+    MF.getProperties().reset(MachineFunctionProperties::Property::IsSSA);
+    MF.getProperties().reset(MachineFunctionProperties::Property::NoVRegs);
+
----------------
arsenm wrote:
> Should implement MachineFunctionPass::getClearedProperties instead of clearing these here
Will do.


================
Comment at: llvm/test/CodeGen/AMDGPU/csr-sgpr-spill-live-ins.mir:17-19
+  ; CHECK-NEXT:   liveins: $sgpr42, $sgpr43, $sgpr46, $sgpr47, $vgpr0
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   $sgpr4_sgpr5 = S_OR_SAVEEXEC_B64 -1, implicit-def $exec, implicit-def dead $scc, implicit $exec
----------------
arsenm wrote:
> Can you precommit a change to add the -NEXTs here
Will do.


================
Comment at: llvm/test/CodeGen/AMDGPU/spill-sgpr-to-virtual-vgpr.mir:26
+    ; GCN-NEXT: [[DEF:%[0-9]+]]:vgpr_32 = IMPLICIT_DEF
+    ; GCN-NEXT: [[V_WRITELANE_B32_:%[0-9]+]]:vgpr_32 = V_WRITELANE_B32 killed $sgpr10, 0, [[V_WRITELANE_B32_]]
+    ; GCN-NEXT: $sgpr10 = V_READLANE_B32 [[V_WRITELANE_B32_]], 0
----------------
arsenm wrote:
> The test checks seem to not capture that these operands are tied
The auto-generator didn't generate the tied-operands correctly. I can hand-modify this test to show the tied operand. It's the simplest case.


================
Comment at: llvm/test/CodeGen/AMDGPU/spill-sgpr-to-virtual-vgpr.mir:58
+    ; GCN-NEXT: {{  $}}
+    ; GCN-NEXT: [[V_WRITELANE_B32_1:%[0-9]+]]:vgpr_32 = IMPLICIT_DEF
+    ; GCN-NEXT: [[V_WRITELANE_B32_1]]:vgpr_32 = V_WRITELANE_B32 killed $sgpr64, 0, [[V_WRITELANE_B32_1]]
----------------
This test is already hand-modified to check the tied operands.


================
Comment at: llvm/test/CodeGen/AMDGPU/spill-sgpr-to-virtual-vgpr.mir:194
+  ; GCN-NEXT:   [[DEF:%[0-9]+]]:vgpr_32 = IMPLICIT_DEF
+  ; GCN-NEXT:   S_CBRANCH_SCC1 %bb.2, implicit killed $scc
+  ; GCN-NEXT: {{  $}}
----------------
arsenm wrote:
> Needs a case where the insert block has no terminators
I couldn't write one successfully. Will try some unstructured flow to force one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124196