[mips] Compute MaxCallFrame size early on

[smaksimovic]

Summary

Use computeMaxCallFrameSize() introduced in D32622
to calculate MaxCallFrameSize after instruction selection.

Test that the value is computed before the Prologue/Epilogue pass
after which MaxCallFrameSize will have been computed anyway.

[smaksimovic]

Correct wrong character starting a comment in test file

[sdardis]

LGTM.

[smaksimovic]

Looking again at the tests reported in the upstream issue I found that with these changes DebugInfo/Mips/dsr-fixed-objects.ll fails still.
The reason being is that the frame register is queried with getFrameRegister() ( which ultimately calls getMaxCallFrameSize() ) inside SelectionDAGISel::runOnMachineFunction() prior to calling finalizeLowering() which calculates maxCallFrameSize.

We could call the function which computes the maxCallFrameSize in MipsDAGToDAGISel::runOnMachineFunction() prior to running selectionDAG which would compute maxCallFrameSize, albeit erroneously.
Computing maxCallFrameSize afterwards finalizeLowering() does get us the correct value, but seems like a hacky way to get around the problem.

[sdardis]

Ok, hold off on committing this for now, I want to check this, as I'd missed the debuginfo tests when reviewing this. Good catch though.

lib/Target/Mips/MipsISelLowering.h

373

Nit: whitespace here.

[sdardis]

+CC'ing @MatzeB for additional insight on what needs to be changed.

Found the bug. What's occurring with the debug information test failures with this patch, is that during the insertion of debug instructions for function arguments, it has to determine which register to assign to the debug instruction, see SelectionDAGISel.cpp:505. Here it asks for the frame pointer register which for some RISC like targets by the size of the frame for the function.

It seems to me that we need to compute the max call frame size before that point (SelectionDAGISel.cpp:505) for targets which use call frame setup and destroy pseudo instructions, this however requires changes for every target which implements finalizeLowering() to compute the maxCallFrameSize.

[MatzeB]

In D42534#990357, @sdardis wrote:

+CC'ing @MatzeB for additional insight on what needs to be changed.

Found the bug. What's occurring with the debug information test failures with this patch, is that during the insertion of debug instructions for function arguments, it has to determine which register to assign to the debug instruction, see SelectionDAGISel.cpp:505. Here it asks for the frame pointer register which for some RISC like targets by the size of the frame for the function.

It seems to me that we need to compute the max call frame size before that point (SelectionDAGISel.cpp:505) for targets which use call frame setup and destroy pseudo instructions, this however requires changes for every target which implements finalizeLowering() to compute the maxCallFrameSize.

Yeah code calling hasFP() early in the process is a real annoyance. You can find me wiggling in the review of https://reviews.llvm.org/D32622 too. At the time it seemed to me like it only influences scheduling heuristic so I went with this hack in the ARM target:

// hasFP ends up calling getMaxCallFrameComputed() which may not be
// available when getPressureLimit() is called as part of
// ScheduleDAGRRList.
bool HasFP = MF.getFrameInfo().isMaxCallFrameSizeComputed()
             ? TFI->hasFP(MF) : true;

instead.

But it looks like you found a bigger problem than the heuristic. If you can find ways for ISel to not do this anymore I would certainly apreciate it!

[sdardis]

In D42534#990829, @MatzeB wrote:

In D42534#990357, @sdardis wrote:

+CC'ing @MatzeB for additional insight on what needs to be changed.

Found the bug. What's occurring with the debug information test failures with this patch, is that during the insertion of debug instructions for function arguments, it has to determine which register to assign to the debug instruction, see SelectionDAGISel.cpp:505. Here it asks for the frame pointer register which for some RISC like targets by the size of the frame for the function.

It seems to me that we need to compute the max call frame size before that point (SelectionDAGISel.cpp:505) for targets which use call frame setup and destroy pseudo instructions, this however requires changes for every target which implements finalizeLowering() to compute the maxCallFrameSize.

Yeah code calling hasFP() early in the process is a real annoyance. You can find me wiggling in the review of https://reviews.llvm.org/D32622 too. At the time it seemed to me like it only influences scheduling heuristic so I went with this hack in the ARM target:

// hasFP ends up calling getMaxCallFrameComputed() which may not be
// available when getPressureLimit() is called as part of
// ScheduleDAGRRList.
bool HasFP = MF.getFrameInfo().isMaxCallFrameSizeComputed()
             ? TFI->hasFP(MF) : true;

instead.

But it looks like you found a bigger problem than the heuristic. If you can find ways for ISel to not do this anymore I would certainly apreciate it!

Sketch solution: Extend the ISD Opcodes to include READ_FP, as SelectionDAG cannot determine ahead of time if function uses a frame pointer (in the general case). The node takes a single argument, the input chain and returns a pair of results, the current value of the frame pointer and the output chain. The semantics of the node are that it returns the value of the frame pointer at that point if the function is using a frame pointer or the current value of stack pointer if the function does not use a frame pointer. The pseudo instruction is then eliminated early during the finalization of instruction selection by replacing it either with a ISD::COPY or by replacing the users of the result ISD::READ_FP with the physreg directly which now can be computed correctly.

ISel can then use the result of this node without needing the maximum frame size during the selection of any block but then ScheduleDAGRRList would still get the wrong result as it only sees one block at a time. We could track if we have seen basic blocks whose instructions contain call setup/destroy instructions which trigger the creation of reserved call frame or some other target specific requirement for a frame pointer. This however would mean that getPressureLimit would see an inconsistent result for a function.

[sdardis]

Marking this as changes required to clear up any confusion.

lib/Target/Mips/MipsISelLowering.h

bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const override {
       // the first 256 for software use (e.g. OpenCL) and treat casts
       // between them as noops.
       return SrcAS < 256 && DestAS < 256;
     }
 
     bool isJumpTableRelative() const override {
       return getTargetMachine().isPositionIndependent();
     }
     
+    void finalizeLowering(MachineFunction &MF) const override;
+
   protected:
     SDValue getGlobalReg(SelectionDAG &DAG, EVT Ty) const;
 
     // This method creates the following nodes, which are necessary for
     // computing a local symbol's address:
     //
     // (add (load (wrapper $gp, %got(sym)), %lo(sym))
     template <class NodeTy>

lib/Target/Mips/MipsISelLowering.cpp

static bool isShiftedMask(uint64_t I, uint64_t &Pos, uint64_t &Size) {
   if (!isShiftedMask_64(I))
     return false;
 
   Size = countPopulation(I);
   Pos = countTrailingZeros(I);
   return true;
 }
 
+void MipsTargetLowering::finalizeLowering(MachineFunction &MF) const {
+  MF.getFrameInfo().computeMaxCallFrameSize(MF);
+  TargetLoweringBase::finalizeLowering(MF);
+}
+
 // The MIPS MSA ABI passes vector arguments in the integer register set.
 // The number of integer registers used is dependant on the ABI used.
 MVT MipsTargetLowering::getRegisterTypeForCallingConv(MVT VT) const {
   if (VT.isVector() && Subtarget.hasMSA())
     return Subtarget.isABI_O32() ? MVT::i32 : MVT::i64;
   return MipsTargetLowering::getRegisterType(VT);
 }
 

lib/Target/Mips/MipsRegisterInfo.cpp

MipsRegisterInfo::getRegPressureLimit(const TargetRegisterClass *RC,
                                       MachineFunction &MF) const {
   switch (RC->getID()) {
   default:
     return 0;
   case Mips::GPR32RegClassID:
   case Mips::GPR64RegClassID:
   case Mips::DSPRRegClassID: {
     const TargetFrameLowering *TFI = MF.getSubtarget().getFrameLowering();
-    return 28 - TFI->hasFP(MF);
+    bool HasFP = MF.getFrameInfo().isMaxCallFrameSizeComputed()
+                 ? TFI->hasFP(MF) : true;
+    return 28 - HasFP;
   }
   case Mips::FGR32RegClassID:
     return 32;
   case Mips::AFGR64RegClassID:
     return 16;
   case Mips::FGR64RegClassID:
     return 32;
   }

test/CodeGen/Mips/maxcallframesize.ll

+; RUN: llc < %s -mtriple=mips-unknown-linux -stop-before=prologepilog | FileCheck %s
+
+# Test that maxCallFrameSize is being computed early on.
+
+@glob = external global i32*
+
+declare void @bar(i32*, [20000 x i8]* byval)
+
+define void @foo([20000 x i8]* %addr) {
+  %tmp = alloca [4 x i32], align 32
+  %tmp0 = getelementptr [4 x i32], [4 x i32]* %tmp, i32 0, i32 0
+  call void @bar(i32* %tmp0, [20000 x i8]* byval %addr)
+  ret void
+}
+
+; CHECK: adjustsStack:    true
+; CHECK: maxCallFrameSize: 20008