[PATCH] D74506: [SystemZ] Support the kernel backchain

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

Yes, this now looks correct to me.  However, we now have duplicated computation between LowerFormalArguments and assignCalleeSavedSpillSlots again, which I don't really like.

I'm wondering whether the best solution wouldn't be to simply make the SystemZFrameLowering::getRegSpillOffset routine take packed stack into account itself.  E.g. something like this:

  unsigned getRegSpillOffset(const MachineFunction &MF, unsigned Reg) const {
      unsigned Offset = RegSpillOffsets[Reg];
      if (usePackedStack(MF)) {
        if (SystemZ::GR64BitRegClass.contains(Reg)) {
          if (!(IsVarArg && !SoftFloat)) {
  	​    // Put all GPRs at the top of the Register save area with packed
  	​    // stack. Make room for the backchain if needed.
  	​    Offset += BackChain ? 24 : 32;
          } else
    ​        Offset = 0;
      }
      return Offset;
  }

and then use that function throughout LowerFormalArguments and assignCalleeSavedSpillSlots ...



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2018
+      " " + Args.getLastArg(options::OPT_mbackchain)->getAsString(Args) +
+      " " + "-mhard-float";
   }
----------------
It's a bit weird to hard-code only "-mhard-float" here.   I guess this is because we may not actually have any "getLastArg" since hard-float is the default?   In that case I guess we might as well hard-code the whole string.


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:503
+      // The back chain is stored topmost with packed-stack.
+      int Offset = usePackedStack(MF) ? 152 : 0;
       BuildMI(MBB, MBBI, DL, ZII->get(SystemZ::STG))
----------------
SystemZMC::CallFrameSize - 8 would be nicer than the hard-coded 152.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1472
+    if (PackedStack && SoftFloat)
+      RegSaveOffset += (BackChain ? 24 : 32);
+
----------------
See main comment about this duplicated logic here.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:3262
+  if (HasPackedStackAttr && !HasBackChain)
+    return DAG.getConstant(0, DL, PtrVT);
+
----------------
Hmm, GCC will always return the address where the back-chain would have been, even if it isn't emitted.  Not sure whether that is very useful, but maybe we should emulate it simply for compatibility reasons ...


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

https://reviews.llvm.org/D74506