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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

OK, just a few more small comments, otherwise looks really good now.   Thanks!



================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:124
+                       ? -SystemZMC::CallFrameSize
+                       : -(SystemZMC::CallFrameSize - StartSPOffset);
   for (auto &CS : CSI) {
----------------
Maybe easier to understand:


```
int CurrOffset = -SystemZMC::CallFrameSize;
if (usePackedStack(MF))
  CurrOffset += StartSPOffset;

```


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:3262
+  if (HasPackedStackAttr && !HasBackChain)
+    return DAG.getConstant(0, DL, PtrVT);
+
----------------
jonpa wrote:
> uweigand wrote:
> > 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 ...
> I thought it might be bad to create two stack slots to the same location (slot of abscent backchain / R15), but it appears to be fine.
> 
> However, if creating that slot in case of no saved regs but with a local area, as in frameaddr-02.ll:fp1f, this ends up actually creating extra stack space for the backchain without using it.
> 
> Not sure what would be best to do in that case if not returning 0...
> 
I see.   I guess it's fine to return 0 in that case then.  (This is anyway not a mode that's used by real-world use cases.)


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1478
+        unsigned Offset = TFL->getRegSpillOffset(MF, SystemZ::ArgFPRs[I]);
         int FI = MFI.CreateFixedObject(8, RegSaveOffset + Offset, true);
         SDValue FIN = DAG.getFrameIndex(FI, getPointerTy(DAG.getDataLayout()));
----------------
I believe this now needs to be -SystemZMC::CallFrameSize + Offset instead of RegSaveOffset + Offset.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:3254
+  // Return null if the back chain is not present.
+  bool HasPackedStackAttr = MF.getFunction().hasFnAttribute("packed-stack");
+  bool HasBackChain = MF.getFunction().hasFnAttribute("backchain");
----------------
We probably shoud use usePackedStack here for consistency.


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

https://reviews.llvm.org/D74506