[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/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);
----------------
arsenm wrote:
> D137574 is in flight to invert the direction, should we land that first / separately?
Alex's patch has landed. But this code is still needed to update the liveness for each instruction as eliminateFrameIndex is called here. 


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


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


================
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;
----------------
arsenm wrote:
> 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 
CurrentVRegSpilled is needed to track the virtual register (Liverange) for which the physical register was assigned. And it is needed only for fast regalloc . We need this mapping to correctly track the WWM spills as RegAllocFast spills/restore the physical registers directly as there is no VRM. This will be appropriately set with the delegate MRI_NoteVirtualRegisterSpill which is inserted in the RegAllocFast spill/reload functions.

SIMachineFunctionInfo is where the delegates are currently handled and I don't have a better place to move it.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:651
+  Register ExecCopyReg = MFI->getSGPRForEXECCopy();
+  if (ExecCopyReg != AMDGPU::NoRegister)
+    reserveRegisterTuples(Reserved, ExecCopyReg);
----------------
arsenm wrote:
> Isn't this always required?
No. They are reserved only if RA inserts any whole wave spill.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124196