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

[Neumann Hon via Phabricator via llvm-commits]

Everybody0523 added inline comments.


================
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))>;
----------------
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.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1504
+      int64_t ArgSPOffset = VA.getLocMemOffset(); 
+      if (Subtarget.isTargetXPLINK64()) {
+        auto &XPRegs = Subtarget.getSpecialRegisters<SystemZXPLINK64Registers>();
----------------
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


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

https://reviews.llvm.org/D114457