Details
- Reviewers
arsenm - Commits
- rG0bcda1a26130: [AMDGPU] Scavenge temp reg for AGPR spill
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
---|---|---|
1358 | I think this should be dealt with in eliminateFrameIndex rather than trying to speculatively add the def here |
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
---|---|---|
1347 | I really don't think trying to guess at the behavior of the allocator is the right way to go | |
1358 | 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? |
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
---|---|---|
1358 | That would require some implementation in all of the allocators... |
AFAIR another reason to request a vreg was that eliminateFrameIndex() does not always have a scavenger available.
Yes, but that's under target control. Having AGPRs could be another condition we could add to enable the scavenger. The alternative without the scavenger is to reserve a register. In this case since it's just a VGPR, the worst case is the emergency stack slot will be used for a temporary spill which as at least more sane than in the SGPR spilling case
llvm/lib/Target/AMDGPU/SIInstructions.td | ||
---|---|---|
674–677 | Can you add a comment explaining UsesTmp? It took me a minute to figure out what the point of this was. Is this accounting for the waitcnts and overly large frame offsets too? I'm not sure it's super important to get this perfect, since BranchRelaxation runs after this but it doesn't hurt to be conservative here | |
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp | ||
823 | !TmpReg | |
llvm/test/CodeGen/AMDGPU/spill-agpr.mir | ||
59 | FYI I have a patch to fix 96-bit spills |
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp | ||
---|---|---|
824 | Should assert on RS in case this ever gets called from PEI and we didn't enable the scavenger |
This is also assuming the allocator. Nothing like this should be based on the opt level