[PATCH] D143759: [AMDGPU] Implement whole wave register spill

[Matt Arsenault via Phabricator via llvm-commits]

arsenm requested changes to this revision.
arsenm added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Target/AMDGPU/SIDefines.h:934
+enum Register_Flag : uint8_t {
+  WWM_REG = 0 // Register operand in a whole-wave mode operation.
+};
----------------
I assumed the value would be 1. Change this to be the pre-shifted values rather than using 1 << flag in the getter?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12934-12940
+  // Reserve the SGPR(s) to save/restore EXEC for WWM spill/copy handling.
+  unsigned MaxNumSGPRs = ST.getMaxNumSGPRs(MF);
+  Register SReg = ST.isWave32()
+                      ? AMDGPU::SGPR_32RegClass.getRegister(MaxNumSGPRs - 1)
+                      : TRI->getAlignedHighSGPRForRC(MF, /*Align=*/2,
+                                                     &AMDGPU::SGPR_64RegClass);
+  Info->setSGPRForEXECCopy(SReg);
----------------
Move this up with the other reserved register logic 


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:549
+
+void SIMachineFunctionInfo::MRI_NotecloneVirtualRegister(Register NewReg,
+                                                         Register SrcReg) {
----------------
Capitalization is weird, Capitalize clone


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:651
+    assert(Reg.isVirtual());
+    if (VRegFlags.inBounds(Reg))
+      VRegFlags[Reg] |= (uint8_t)1 << Flag;
----------------
Why are the inbounds checks needed? The analagous MRI functions do not tolerate invalid register values 


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:656-657
+  bool checkFlag(Register Reg, uint8_t Flag) const {
+    if (Reg.isPhysical())
+      return false;
+
----------------
This should assert (like setFlag already does)


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:659
+
+    return VRegFlags.inBounds(Reg) && VRegFlags[Reg] & ((uint8_t)1 << Flag);
+  }
----------------
Ditto, why range check?


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:2149-2150
+      if (IsWWMRegSpill)
+      TII->insertScratchExecCopy(*MF, *MBB, MI, DL, MFI->getSGPRForEXECCopy(),
+                                 RS->isRegUsed(AMDGPU::SCC));
+
----------------
Braces and indent under if


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:2158
+      if (IsWWMRegSpill)
+      TII->restoreExec(*MF, *MBB, MI, DL, MFI->getSGPRForEXECCopy());
+
----------------
Indent off


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143759