This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Scavenge temp reg for AGPR spill
ClosedPublic

Authored by rampitec on Aug 4 2020, 11:48 AM.

Diff Detail

Event Timeline

rampitec created this revision.Aug 4 2020, 11:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2020, 11:48 AM
rampitec requested review of this revision.Aug 4 2020, 11:48 AM
arsenm added inline comments.Aug 4 2020, 12:37 PM
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

arsenm added inline comments.Aug 4 2020, 12:38 PM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
1347

This is also assuming the allocator. Nothing like this should be based on the opt level

1469–1470

This should enter block backwards

rampitec updated this revision to Diff 283005.Aug 4 2020, 12:52 PM
rampitec marked an inline comment as done.

Use enterBasicBlockEnd().

rampitec added inline comments.Aug 4 2020, 12:55 PM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
1347

Any ideas how to check which allocator we use?

1358

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.

arsenm added inline comments.Aug 4 2020, 1:02 PM
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?

rampitec added inline comments.Aug 4 2020, 1:07 PM
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.

arsenm added a comment.Aug 4 2020, 1:22 PM

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

rampitec updated this revision to Diff 283039.Aug 4 2020, 3:25 PM
rampitec marked 4 inline comments as done.

Moved scavenging into eliminateFrameIndex().

rampitec updated this revision to Diff 283041.Aug 4 2020, 3:29 PM

Updated test checks.

rampitec retitled this revision from [AMDGPU] Scavenge a temp register for AGPR spill in fast RA to [AMDGPU] Scavenge temp reg for AGPR spill.Aug 4 2020, 3:44 PM
rampitec updated this revision to Diff 283050.Aug 4 2020, 3:55 PM

Removed SI_SPILL_AGPR multiclass.

arsenm added inline comments.Aug 5 2020, 10:52 AM
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

arsenm added inline comments.Aug 5 2020, 10:54 AM
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

rampitec updated this revision to Diff 283311.Aug 5 2020, 11:19 AM
rampitec marked 3 inline comments as done.
arsenm accepted this revision.Aug 5 2020, 11:23 AM
This revision is now accepted and ready to land.Aug 5 2020, 11:23 AM
This revision was automatically updated to reflect the committed changes.