[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/SIFrameLowering.cpp:1349-1350
                                                 TRI->isAGPR(MRI, VReg))) {
-            // FIXME: change to enterBasicBlockEnd()
-            RS->enterBasicBlock(MBB);
+            RS->enterBasicBlockEnd(MBB);
+            RS->backward(MI);
             TRI->eliminateFrameIndex(MI, 0, FIOp, RS);
----------------
D137574 is in flight to invert the direction, should we land that first / separately?


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.h:628
 
+  bool isWWMRegSpillOpcode(uint16_t Opcode) const {
+    return Opcode == AMDGPU::SI_SPILL_WWM_V32_SAVE ||
----------------
static?


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:63
   void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addRequired<MachineDominatorTree>();
     AU.setPreservesAll();
----------------
Is this introducing a new computation in the pass pipeline (I assume not since I don't see a pass pipeline test update)


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:490-495
+  // Map a physical register into the virtual register it currently represents.
+  // This field is relevant only during RegAllocFast and will be reset to zero
+  // all other times. It is needed while spilling a physical register (happens
+  // with RegAllocFast) that we need to access the VRegFlags for the original
+  // virtual register.
+  Register CurrentVRegSpilled;
----------------
I don't like having state here for a single operation that's happening in one pass and isn't valid for multiple uses. I don't really understand how this is being set and passed around 


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:688
+  void setFlag(Register Reg, uint8_t Flag) {
+    assert(Register::isVirtualRegister(Reg));
+    if (VRegFlags.inBounds(Reg))
----------------
Reg.isVirtual()


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:694
+  bool checkFlag(Register Reg, uint8_t Flag) const {
+    if (!Register::isVirtualRegister(Reg)) {
+      // See if a virtReg is available for the physReg. If found, check the
----------------
Reg.isVirtual()


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:651
+  Register ExecCopyReg = MFI->getSGPRForEXECCopy();
+  if (ExecCopyReg != AMDGPU::NoRegister)
+    reserveRegisterTuples(Reserved, ExecCopyReg);
----------------
Isn't this always required?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124196