This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Spill CSR VGPR which is reserved for SGPR spills
ClosedPublic

Authored by kerbowa on Jul 13 2020, 4:40 PM.

Details

Summary

A CSR VGPR being reserved for SGPR spills could be clobbered if there were no
free lower VGPR's available. Create a stack object so that it will be spilled
in the prologue.

Update the test to include a stack object so that a VGPR will be pre-reserved.

Diff Detail

Event Timeline

kerbowa created this revision.Jul 13 2020, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2020, 4:40 PM
kerbowa updated this revision to Diff 278243.Jul 15 2020, 10:45 AM

Workaround MIR emitter issue.

arsenm added inline comments.Jul 15 2020, 11:37 AM
llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
361

I think checking for isEntryFunction is redundant with checking for CSRegs. Why does this need to worry about calls? This will miss the tail call case.

Can you also comment this?

llvm/test/CodeGen/AMDGPU/reserve-vgpr-for-sgpr-spill.ll
19–22

Can you also add a test that only has a tail call?

kerbowa marked an inline comment as done.Jul 23 2020, 7:52 PM
kerbowa added inline comments.
llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
361

The logic is the same for other CSRegs being reserved for SGPR spills.

I'm not sure in what situation this might miss the tail call case? If there is a tail call and we don't clobber any SGPRs there will be no reserved VGPR and no unnecessary spill.

kerbowa updated this revision to Diff 280338.Jul 23 2020, 11:42 PM
kerbowa marked an inline comment as not done.

Create the stack object in lowerShiftReservedVGPR. Also handle this reserved VGPR not being live-in to all BB. Add more tests. Cleanup some of the existing logic.

LGTM. Please wait for @arsenm 's review.

arsenm accepted this revision.Jul 28 2020, 11:07 AM

LGTM with nit. I really need to get back to the register allocation split patches

llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp
241

Extra whitespace change

This revision is now accepted and ready to land.Jul 28 2020, 11:07 AM
This revision was automatically updated to reflect the committed changes.