[PATCH] D85234: [AMDGPU] Scavenge a temp register for AGPR spill in fast RA

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:1350
+    // deal with newly added virtual registers.
+    if (MF->getTarget().getOptLevel() == CodeGenOpt::None) {
+      std::unique_ptr<RegScavenger> RS(new RegScavenger());
----------------
rampitec wrote:
> arsenm wrote:
> > This is also assuming the allocator. Nothing like this should be based on the opt level
> Any ideas how to check which allocator we use?
I really don't think trying to guess at the behavior of the allocator is the right way to go


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:1361
+      MachineRegisterInfo &MRI = MF->getRegInfo();
+      Tmp = MRI.createVirtualRegister(&AMDGPU::VGPR_32RegClass);
+    }
----------------
rampitec wrote:
> arsenm wrote:
> > I think this should be dealt with in eliminateFrameIndex rather than trying to speculatively add the def here
> It seems more robust to use the allocator rather than scavenger. We may need scavenger at -O0, but at least can do it better at a normal opt level.
I'm not sure it's necessarily correct in greedy. You're not updating the live intervals here for example, and am not sure it will end up working out always.

In general you need a reserved register to deal with spilling. Maybe we could add some mechanism to inform the allocator when a temp reg with another class is needed for spilling?


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

https://reviews.llvm.org/D85234