This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Stash GPR to VSR if emergency spill slot is not reachable
ClosedPublic

Authored by nemanjai on May 3 2022, 4:27 AM.

Details

Summary

When removing frame indices on PowerPC, we need to scavenge a GPR to materialize a large constant if the stack offset for the spill/reload cannot be reached by a D-Form instruction. However, in a perfect storm of conditions, we may not have GPR's available to scavenge, thereby requiring an emergency spill. If such an emergency spill also needs to be spilled to a location with a large offset, it would itself require register scavenging thereby creating an infinite loop.

This patch detects when the scavenger cannot scavenge a register and the spill/reload is to a location with a large offset. It then stashes a GPR into a VSR so that it can use the GPR to materialize the constant (rather than scavenging a GPR).

Fixes: https://github.com/llvm/llvm-project/issues/52894

Diff Detail

Event Timeline

nemanjai created this revision.May 3 2022, 4:27 AM
nemanjai requested review of this revision.May 3 2022, 4:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2022, 4:27 AM

If such an emergency spill also needs to be spilled to a location with a large offset

This shouldn't happen; the emergency spill slot should be placed in a location with a small offset from the frame/stack pointer.

If it's impossible to allocate an emergency spill slot close to the stack pointer, I think we should force a frame pointer, and the emergency spill slot should be allocated close to that. See, for example, AArch64FrameLowering::hasFP().

pkubaj accepted this revision.May 4 2022, 4:08 AM

Applying this patch fixes the original issue (building audio/calf-lv2 port from FreeBSD ports tree on powerpc64le).

This revision is now accepted and ready to land.May 4 2022, 4:08 AM

If such an emergency spill also needs to be spilled to a location with a large offset

This shouldn't happen; the emergency spill slot should be placed in a location with a small offset from the frame/stack pointer.

If it's impossible to allocate an emergency spill slot close to the stack pointer, I think we should force a frame pointer, and the emergency spill slot should be allocated close to that. See, for example, AArch64FrameLowering::hasFP().

That certainly works in cases where the problem is a dynamic allocation in the stack frame. However, the issue here is that the fixed stack part of the frame (i.e. the parameter save area mandated by the ABI) is so large that all stack allocations are more than 32kB away from the stack pointer.

If such an emergency spill also needs to be spilled to a location with a large offset

This shouldn't happen; the emergency spill slot should be placed in a location with a small offset from the frame/stack pointer.

If it's impossible to allocate an emergency spill slot close to the stack pointer, I think we should force a frame pointer, and the emergency spill slot should be allocated close to that. See, for example, AArch64FrameLowering::hasFP().

That certainly works in cases where the problem is a dynamic allocation in the stack frame. However, the issue here is that the fixed stack part of the frame (i.e. the parameter save area mandated by the ABI) is so large that all stack allocations are more than 32kB away from the stack pointer.

Are you saying that even though we have a frame pointer, it's impossible to allocate a stack slot close to the frame pointer? That seems unlikely to me; normal stack layout rules should allow allocating as much space as we need close to the frame pointer. Depending on how the stack is laid out, I guess it's possible that normal spill slots are never close enough to the frame pointer. But if we really need it, we could allocate the emergency spill slot next to the callee-saved registers. (This currently isn't implemented, but shouldn't be complicated.)

...

Are you saying that even though we have a frame pointer, it's impossible to allocate a stack slot close to the frame pointer? That seems unlikely to me; normal stack layout rules should allow allocating as much space as we need close to the frame pointer. Depending on how the stack is laid out, I guess it's possible that normal spill slots are never close enough to the frame pointer. But if we really need it, we could allocate the emergency spill slot next to the callee-saved registers. (This currently isn't implemented, but shouldn't be complicated.)

To be more precise, I am not saying that this is fundamentally unimplementable but that with the current implementation of how we treat the stack pointer and frame pointer, the frame pointer does not help.
When a function maintains a frame pointer on PowerPC, the frame pointer is equal to the stack pointer until a dynamic allocation changes the stack pointer. After a dynamic allocation, obviously the difference between the two is the (aligned) size of the allocation.
Stack pointer update is the first thing that happens, then the copy to the frame pointer, CSR saves, etc. Given that implementation, if a stack slot is not close enough to the stack pointer, it is not close enough to the frame pointer - and having a frame pointer does not help at all.
Of course, if we changed the implementation to have the frame pointer point to the caller's stack frame, we could address things with a negative offset that accounts for the stack frame size (the static stack frame prior to the dynamic allocation).

This revision was landed with ongoing or failed builds.Oct 13 2022, 7:09 AM
This revision was automatically updated to reflect the committed changes.
foad added a subscriber: foad.May 15 2023, 11:24 AM
foad added inline comments.
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
1635

FYI you can arrange for RS to always be non-nullptr here, by overriding PPCRegisterInfo::requiresFrameIndexReplacementScavenging to return true. But that changes the code generated for the tests you added here, to the point that I'm not even sure if they're still testing the right thing.