[PATCH] D70821: [SystemZ] Implement the packed stack layout

Fri Nov 29 02:43:15 PST 2019

jonpa updated this revision to Diff 231502.
jonpa added a comment.

>   I don't think this approach will really do everything -mpacked-stack is supposed to do. Fundamentally, with the packed stack layout, only some N (< 160) bytes of the incoming register save area are reserved for register save slots, instead of all 160 bytes. Everything below "(incoming SP) + 160 - N" is supposed to be freely usable for any use, including FPR/VR save slots and outgoing function args / register save area, but also just local stack variables and reload spill slots in general.

I think it does achieve that... The local area offset is not changed, but my intention is to instead move up all the slots in the local area with the amount that was packed:

- First, the GPRs are stored topmost and then any (anyregcc) FP arg regs below them in the reg save area. The amount used of the reg save area is tracked during this with ZFI->incRegSaveAreaUsage(). So any registers which already belong in the reg save area still go there, but with new offets. The amount used is then not 160, but ZFI->getRegSaveAreaUsage().

- The stack is then grown by this amount instead of CallFrameSize. This means that we should have just enough space to pass over to callee.

- When Frame indexes are replaced, getFrameIndexReference() still returns the right offsets for all the fixed objects (incoming stack args and regs in the reg save area), since the stack size is part of that computation. The outgoing stack arguments do not have any frame objects, they are hard coded to begin at SP+160 at the call site, so the remaining objects are those that are non-fixed and all now ordinarily begin at CFA-160 (Local Area Offset). So by simply moving up any of these non-fixed objects by the amount that was packed/saved in the newly derived SystemZFrameLowering::getFrameIndexReference(), we end up with the 160 bytes bottom most for callee...

This for me seems like the most natural way to extend the current machinery while changing as little as possible...

> (Conversely, if the general case were addressed, you actually wouldn't have to implement anything special for those special cases.

It would have been simpler to just spill everything without any fixed offets, but since we want to do STMG I wasn't really sure that was a good idea...

> So ideally, you'd want to set LocalAreaOffset to -N (where N depends on the number of GPRs to be saved) instead of -160 for the packed stack layout. Unfortunately, I believe the LocalAreaOffset mechanism doesn't support a variable value for this field. One option would be to find (or create) some way to instantiate a per-function FrameLowering. Another way might be to just always set the LocalAreaOffset to 0 and then force common code to still leave the register save area fully (non-packed layout) or partially (packed layout) empty by placing fixed frame objects there (cf. the logic in PEI::calculateFrameObjectOffsets).

Perhaps we could just set a new smaller LAO value once we know it in emitPrologue(), instead of adding the amount for each offset in SystemZFrameLowering::getFrameIndexReference()?

Getting rid of the LAO and creating fixed objects as needed also sounds like a nice idea. That way those offsets wouldn't have to be changed again as per this current patch. But then I think we would have to first rework all the places that assume the LAO as per the patch I first suggested...

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70821/new/

https://reviews.llvm.org/D70821

Files:
  llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.h
  llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
  llvm/lib/Target/SystemZ/SystemZFrameLowering.h
  llvm/lib/Target/SystemZ/SystemZMachineFunctionInfo.h
  llvm/test/CodeGen/SystemZ/frame-22.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70821.231502.patch
Type: text/x-patch
Size: 12140 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191129/b98132c4/attachment.bin>