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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
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) {
----------------
jonpa wrote:
> 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.
Yes, we'd use something like LowSpillGPR/HighSpillGPR/SpillGPROffset and LowRestoreGPR/HighRestoreGPR/RestoreGPROffset (could also be grouped instead of having separate accessors) to indicate the different register ranges & offset for the STMG vs. LMG.

Basically, have assignCalledSavedSpillSlots fully prepare the arguments of STMG and LMG, and then have spillCalleeSavedRegister/restoreCalleeSavedRegister just emit those instructions.

The reason I'm concerned is that the logic below really makes assumptions about how the LowGPR and the offset are associated.  In fact, it would not work with the packed layout (which is why this is disabled above for now ...).  It just seems cleaner to have this in one place.


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:48
 
+bool SystemZFrameLowering::usePackedStack(MachineFunction &MF) const {
+  bool HasPackedStackAttr = MF.getFunction().hasFnAttribute("packed-stack");
----------------
Why does this even need to be a SystemZFrameLowering member function?  Could be just static in this file ...


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.h:12
 
+#include "SystemZMachineFunctionInfo.h"
 #include "llvm/ADT/IndexedMap.h"
----------------
What is this needed for?


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

https://reviews.llvm.org/D70821