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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

This now looks all functionally correct to me, just a few more cosmetic issues ...



================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:51
+                          0, Align(8), false /* StackRealignable */),
+      RegSpillOffsets(0) {
   // Due to the SystemZ ABI, the DWARF CFA (Canonical Frame Address) is not
----------------
jonpa wrote:
> uweigand wrote:
> > Why is the RegSpillOffsets change above needed?
> I'm not sure this is needed, but since I am using this table also with CSRs that are not in it, I wanted to make sure that zero is returned in those cases.
> 
> IIUC, nullVal_ will by this be initialized to 0, which will then be the initial value of all the elements. Perhaps this is redundant with a default constructor for unsigned?
Well, it can't hurt, so this is fine with me.


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:129
+    unsigned Size = TRI->getSpillSize(*RC);
+    CurrOffset -= Size;
+    int FrameIdx = MFFrame.CreateFixedSpillStackObject(Size, CurrOffset);
----------------
This ignores alignment.  That's in fact OK here since for all register save slots we only require 8-byte alignment and all register sizes are a multiple of 8, so this is always OK.  An assert (or at least a comment) would still be good.


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:228
   // be included by the loop above, but we also need to handle the
   // call-clobbered argument registers.
   if (IsVarArg) {
----------------
I still don't quite like how like logic is now split between two routines.  At the very least, the comment needs to be updated (there no longer is any "loop above").  Even better would be to find a way to move the vararg handling to assignCalleeSavedSpillSlots as well ...


================
Comment at: llvm/lib/Target/SystemZ/SystemZMachineFunctionInfo.h:15
 #include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/IR/Function.h"
 
----------------
Having to add all these includes here seems to indicate a bit of a layering violation.  This is about the  getOrCreateFramePointerSaveIndex routine below?  I think that should probably be a SystemZFrameLowering function instead ...


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

https://reviews.llvm.org/D70821