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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added a comment.

In D70821#1766858 <https://reviews.llvm.org/D70821#1766858>, @uweigand wrote:

> > processFunctionBeforeFrameFinalized() should now be correct but it is not giving the exact same results on SPEC.
>
> I believe the original processFunctionBeforeFrameFinalized computation was correct:
>
>   // Get the size of our stack frame to be allocated ...
>   uint64_t StackSize = (MFFrame.estimateStackSize(MF) +
>                         SystemZMC::CallFrameSize);
>   
>   
>
> At this point, estimateStackSize is supposed to return the size of the objects allocated in the local area, **excluding** both incoming and outgoing register save areas.   We add back the size of the outgoing register save area (which we are certain to need in all nontrivial cases).
>
> > MFFrame.estimateStackSize(MF) adds 56 to the stack size based on the -56 offset for %R13 <https://reviews.llvm.org/source/pstl/>. This shouldn't be done, since CallFrameSize is added later...
>
> This should not have happened in the old code because the 56 was within the "LocalAreaOffset" and therefore did not count.  It is true that with the new logic this will have to change now ...
>
>   // ... and the maximum offset we may need to reach into the
>    // caller's frame to access the save area or stack arguments.
>    int64_t MaxArgOffset = SystemZMC::CallFrameSize;
>    for (int I = MFFrame.getObjectIndexBegin(); I != 0; ++I)
>      if (MFFrame.getObjectOffset(I) >= 0) {
>        int64_t ArgOffset = SystemZMC::CallFrameSize +
>                            MFFrame.getObjectOffset(I) +
>                            MFFrame.getObjectSize(I);
>        MaxArgOffset = std::max(MaxArgOffset, ArgOffset);
>      }
>   
>   
>
> Given the above, we know current SP + StackSize suffices to reach the bottom of the incoming register save area.  MaxArgOffset is now supposed to cover everything we may need to reach **beyond** that.  This includes everything within the incoming register save are (as we might need to restore from there), and all the incoming arguments.
>
> Now, the difference with the new logic is that estimateStackSize will include the **incoming** register save area already (all of it in the default case, the parts that are used in the -mpacked-stack case).  Therefore, we need to change the computation of MaxArgOffset, exactly as your patch already does.  Everything else should be the same.
>
> Therefore, I'm not sure why you're seeing any difference in results -- it should be exactly the same.


F10984820: tc_estimatestacksize.ll <https://reviews.llvm.org/F10984820>
llc -mtriple=s390x-linux-gnu ./tc_estimatestacksize.ll -o -

This is the reduced test case I was looking at. It seems that PEI::calculateFrameObjectOffsets() is taking the Local Area Offset into account, while in fact the very "similar" MFFrame.estimateStackSize() in fact does not (but probably should).

On entry to SystemZFrameLowering::processFunctionBeforeFrameFinalized(), the MF looks like

  (gdb) p MF.dump()
  # Machine code for function scan_one_insn: NoPHIs, TracksLiveness, NoVRegs
  Frame Objects:
    fi#-3: size=8, align=8, fixed, at location [SP+120]
    fi#-2: size=8, align=8, fixed, at location [SP+112]
    fi#-1: size=8, align=8, fixed, at location [SP+104]
    fi#0: size=240, align=8, at location [SP+160]
    fi#1: size=120, align=4, at location [SP+160]
    fi#2: size=240, align=8, at location [SP+160]
    fi#3: size=3120, align=4, at location [SP+160]

The allocas occupy 3720 bytes, and there are fixed frame objects for %R13 <https://reviews.llvm.org/source/pstl/>-%R15 <https://reviews.llvm.org/source/zorg/>. MachineFrameInfo::estimateStackSize() starts with Offset = 0, then checks the greatest fixed offset, and then adds the remaining object sizes, so instead of 3720 (like you expected), it returns 3776...


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

https://reviews.llvm.org/D70821