[PATCH] D114457: [z/OS] Implement prologue and epilogue generation for z/OS target.

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:169
 def CSR_SystemZ_XPLINK64 : CalleeSavedRegs<(add (sequence "R%dD", 7, 15),
+                                                (sequence "R%dD", 4, 4),
                                                 (sequence "F%dD", 15, 8))>;
----------------
Everybody0523 wrote:
> uweigand wrote:
> > I'm wondering if it wouldn't be more straightforward to handle this in determineCalleeSaves, in the place where the frame pointer register is handled?
> That's what this line is for. Adding a register to SavedRegs in determineCalleeSaves doesn't do anything unless that register is marked as a callee-saved register here.
Ah, I see.  This is OK then.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1504
+      int64_t ArgSPOffset = VA.getLocMemOffset(); 
+      if (Subtarget.isTargetXPLINK64()) {
+        auto &XPRegs = Subtarget.getSpecialRegisters<SystemZXPLINK64Registers>();
----------------
Everybody0523 wrote:
> uweigand wrote:
> > It's a bit ugly, but I'd be OK with it for now.  It would be good to add a FIXME saying that ideally the call frame size should have already been included in the offset.
> So originally I actually included it in the offset, but I found that it would force similar ugliness in the LowerCall routine [[ https://llvm.org/doxygen/SystemZISelLowering_8cpp_source.html#l01719 | here ]].
> 
> As I understand it, if the call frame size is included in the offset just for XPLINK, then in the location I linked above we would have to only add the call frame size for ELF targets, since the call frame size is already included. ELF callees instead rely on `SystemZELFFrameLowering::getFrameIndexReference` to jump over the call frame size. 
> 
> So I think if we were to add each target's call frame size to the offset, we could actually eliminate the override of getFrameIndexReference for ELF as well. Please correct me if I'm mistaken about the ELF code though. 
> 
> But with that said, I'd prefer not to touch the ELF code in this patch so if you're OK with it for now I'll just leave it as a FIXME
Indeed, this is another place where the current abstractions break down.   I think the proper fix will be to change that line to simply:
```
unsigned Offset = Regs->getStackPointerBias() + VA.getLocMemOffset();
```
and then set StackPointerBias to 160 for ELF.   Those 160 bytes are really a stack bias on ELF - they're part of the callee's stack frame, but always allocated by the caller.

But all this should be done in a follow-up patch.  For now, this patch LGTM as is.


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

https://reviews.llvm.org/D114457