[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/SIFrameLowering.cpp:1297-1299
+    SmallVector<int> SFI;
+    RS->getScavengingFrameIndices(SFI);
+    if (SFI.empty()) {
----------------
I don't understand why you would need to check this


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1122
+    MachineInstrBuilder LastI;
+    for (unsigned I = 0; I < 2; I++) {
+      // Mark the second store as kill
----------------
I believe you don't need to split this when using scratch instructions


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1139-1140
+                                             RegScavenger *RS, bool IsLoad,
+                                             bool UseKillFromMI,
+                                             bool IsKill) const {
+  MachineBasicBlock *MBB = MI->getParent();
----------------
I don't follow this UseKillFromMI vs. isKill. Just use the isKill?


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1337-1343
+    if (SFI.empty()) {
+      int ScavengeFI = MF->getFrameInfo().CreateStackObject(
+          getSpillSize(AMDGPU::SGPR_32RegClass),
+          getSpillAlign(AMDGPU::SGPR_32RegClass), false);
+      RS->addScavengingFrameIndex(ScavengeFI);
+      TmpVGPRIndex = ScavengeFI;
+    } else {
----------------
I don't really like adding this here on demand. You need to be sure this is called after frame finalization. This should be created up front


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1348
+    // Check if TmpVGPR is currently live according to LLVM liveness info
+    RegScavenger TmpRS;
+    TmpRS.enterBasicBlock(*MBB);
----------------
There shouldn't be any temporary reg scavenger created locally. Also, using forward scavenging is deprecated


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1466
+    int TmpVGPRIndex;
+    if (SFI.empty()) {
+      int ScavengeFI = MF->getFrameInfo().CreateStackObject(
----------------
Ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96336