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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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
----------------
Seems worthwhile for this to be its own real function instead of a lambda


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:315
+          auto PrevInsertPt = I->second;
+          MachineBasicBlock *DomMBB = PrevInsertPt->getParent();
+          if (DomMBB == MBB) {
----------------
This could be the end iterator


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:321
+            // if we insert any new spill in it. Check if they dominate only for
+            //  the first spill in case if multiple spills are inserted for the
+            // frame index.
----------------
Typo " in case if multiple spills"


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:366
+          BuildMI(*InsertPt->getParent(), *InsertPt, InsertPt->getDebugLoc(),
+                  TII->get(AMDGPU::IMPLICIT_DEF), Reg);
+      if (LIS) {
----------------
It might be worth adding a target comment flag for this implicit def to comment it's for SGPR spilling


================
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);
+
----------------
Should implement MachineFunctionPass::getClearedProperties instead of clearing these here


================
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
----------------
Can you precommit a change to add the -NEXTs here


================
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
----------------
The test checks seem to not capture that these operands are tied


================
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: {{  $}}
----------------
Needs a case where the insert block has no terminators


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124196