This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Enable base pointer.
ClosedPublic

Authored by cdevadas on Apr 24 2020, 9:08 AM.

Details

Summary

When the callee requires a dynamic stack realignment,
it is not possible to correctly access the incoming
stack arguments using the stack pointer. We reserve a
base pointer in such cases to access the function arguments
inside the callee. The base pointer will hold the incoming
stack pointer value before any kind of delta added to it.

Diff Detail

Event Timeline

cdevadas created this revision.Apr 24 2020, 9:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2020, 9:09 AM
arsenm added inline comments.Apr 24 2020, 4:23 PM
llvm/docs/AMDGPUUsage.rst
3875–3884

This isn't supposed to be an ABI visible thing, so I'm not sure if this belongs here. The note about replacing it with a 0 immediate offset doesn't make sense too

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
110–121

I don't think this case is stressed in the tests, but also don't think we should handle this case here. We assume all SGPR->VGPR spills have been resolved in SILowerSGPRSpills. Trying to get an additional VGPR to spill in PEI is less reliable

646

Register() instead of NoRegister

676–678

Should do a single loop over the function with the other case above (also may require .sortUniqueLiveIns)

885
  • BasePointerSaveIndex is probably the normal way to get the value
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
116

Missing space after comma

122

Can this go in the header or is is the register enum not defined there?

311

This is a unneeded check

llvm/test/CodeGen/AMDGPU/stack-realign.ll
163

Should add a test explanation comment

203

Can you note the BP is saved/restored in an SGPR

cdevadas marked 2 inline comments as done.Apr 26 2020, 10:13 AM
cdevadas added inline comments.
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
885

Here getValue() is required. It is of type Optional<int>.

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
122

Right, the enum isn't defined there.

cdevadas updated this revision to Diff 260175.Apr 26 2020, 10:43 AM
cdevadas edited reviewers, added: arsenm; removed: arsen.

Incorporated the suggestions.

scott.linder added inline comments.Apr 28 2020, 10:56 AM
llvm/docs/AMDGPUUsage.rst
6715

s/requie/requires/

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
795–798

Is this comment accurate anymore? I think it can go back where it was with "base pointer" just changed to "frame pointer".

A new comment for the "base pointer" would need to talk about referencing function arguments, not locals.

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
118

Is it possible to have some mechanism for only setting up the BP when it is actually used? Looking at X86 it seems like they don't do this, so I am OK with us not doing it either, just wondering if it is feasible.

llvm/test/CodeGen/AMDGPU/callee-frame-setup.ll
302

This is an example of where we have dynamic realignment but don't actually use the BP to refer to a fixed index. Again, being correct is more important, but if it is feasible it seems nice to only set this up when it is used.

cdevadas marked 2 inline comments as done.Apr 30 2020, 2:09 AM
cdevadas added inline comments.
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
795–798

I don't think we need to stress it out for the FP.
This is a usual comment for BP in the frame lowering file for each target. Sure, 'locals' here is misleading. I will change it to function arguments.

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
118

It sounds good to me. We can enable it only when there is a fixed-frame.
It can be easily done with an additional check, MFI.getNumFixedObjects() != 0

cdevadas updated this revision to Diff 261479.May 1 2020, 9:45 AM

Incorporated the suggestions.

arsenm added inline comments.May 1 2020, 11:29 AM
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
110–121

I still think this case won't work correctly. This should probably be replaced with report_fatal_error

885

Yes, and you can use operator * instead of getValue

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
1059–1061

Swap the order of these checks

arsenm added inline comments.May 4 2020, 7:16 AM
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
109

This should early return if findScratchNonCalleeSaveRegister and reduce the indentation for the rest of the function

679

Make the small size 2 since that's all that's used

cdevadas updated this revision to Diff 261872.May 4 2020, 10:50 AM

Rebase + Suggestions.

arsenm added inline comments.May 4 2020, 12:31 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
118

I think getNumFixedObjects is not the right check here. That won't deal with the case where we have dead stack objects (as is common from eliminated SGPR spils)

cdevadas marked an inline comment as done.May 8 2020, 1:08 PM
cdevadas added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
118

It would be good to have a straight forward check here to see if there is any fixed-frame and getNumFixedObjects would do that.

arsenm added inline comments.May 11 2020, 3:01 PM
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
744

Don't need the parentheses

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
118

It doesn't, because for us it's full of litter from SGPR spills. This will be overly conservative, and trigger when we have no actual stack objects. Is this only called after the frame is finalized?

The way the x86 version expresses this makes more sense to me:
ister.

bool CantUseFP = needsStackRealignment(MF);
return CantUseFP && CantUseSP(MFI);

This might work for us the same?

cdevadas marked an inline comment as done.May 12 2020, 8:38 AM
cdevadas added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
118

There are calls to hasBasePointer() early during SIIselLowering and it is not always called after the frame is finalized.

It doesn't work in the way that we wanted if we include the function CantUseSP().
The condition 'CantUseFP && CantUseSP'
enables BP when there is dynamic re-align and have either variable-sized objects or opaque SP adjustment.
The dynamic realign alone doesn't qualify to have BP.

arsenm accepted this revision.May 14 2020, 9:32 AM
This revision is now accepted and ready to land.May 14 2020, 9:32 AM
scott.linder accepted this revision.May 14 2020, 11:43 AM

This will require some changes to the CFI, although previously this case was broken anyway so it does not regress the unwind information. I think we will just need to define the CFA relative to the base pointer (if it is actually set up) or the stack pointer (if realignment is needed, but the base pointer is not).

This revision was automatically updated to reflect the committed changes.