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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:129
+    unsigned Size = TRI->getSpillSize(*RC);
+    CurrOffset -= Size;
+    int FrameIdx = MFFrame.CreateFixedSpillStackObject(Size, CurrOffset);
----------------
uweigand wrote:
> 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.
Added assert.


================
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) {
----------------
uweigand wrote:
> 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 ...
I just updated the comment for now.
It kind of makes a little sense to me to have the modification of LowGPR and StartSPOffset as it is in spillCalleeSavedRegisters() since this is only a local modification while the unmodified values are used later by restoreCalleeSavedRegisters().
If that was moved into assignCalleeSavedSpillSlots, it seems we would either have to extend SystemZMachineFunctionInfo with e.g. getLowSavedGPR_Varargs() etc, or perhaps do some handling in restoreCalleeSavedRegisters instead.


================
Comment at: llvm/lib/Target/SystemZ/SystemZMachineFunctionInfo.h:15
 #include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/IR/Function.h"
 
----------------
uweigand wrote:
> 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 ...
Moved the two new methods to SystemZFrameLowering (which both needed the includes).


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

https://reviews.llvm.org/D70821