[AArch64] Fix bug in large stack spill slot handling (PR27717)

[gberry]

Summary

Fix bug in MachO path where a frame index offset would not be reserved
for handling large frames when an extra non-used callee-save register
was saved. In the case where the extra register is reserved or not a
GPR (e.g. %FP in the MachO case), this would lead to the register
scavenger later failing when called from PrologEpilogInserter.

[gberry]

I should add that I don't have a way to test the MachO path other than lit tests, so any additional testing would be appreciated.

[asbirlea]

Tested on my end. This fixes Bug 27717.

[rengolin]

This makes sense to me. Tim?

[asbirlea]

Does it make sense to also include the testcase from the bugreport? It's fairly large, but anything smaller and the issue doesn't come up (bugpoint wasn't very helpful to reduce it further).

[gberry]

I spent some time trying to come up with a smaller test case without much luck. The problem is you need to generate enough spill code that the spill slot address offset doesn't fit in the load/store offset field, and have all GPRs live at the point where the too large spill offset occurs. I couldn't come up with a way to do that, though perhaps others have some idea here.

I could add the original test case, but it seems like it would be fairly fragile in that later changes in register allocation, etc. could easily make it no longer test what it was intended to test.

[asbirlea]

I know what you mean, I spend quite a bit of time to get the original test down to that size in the first place.
FYI: the original test case does not do test anything meaningful, it's only meant to crash on this particular bug.

[rengolin]

Big tests are not the problem, but unstable tests are, especially around register allocation and scheduler. It could put people off-track in the future. I'm ok with this not having a test, too.

[asbirlea]

I'm ok either way. Geoff is right that later changes in reg allocator may make the test no longer exhibit the issue.
Waiting on Tim's input.

[t.p.northover]

I think MIR-level tests are probably the long-term path for FrameLowering. I'm OK with skipping it on this occasion though.

lib/Target/AArch64/AArch64FrameLowering.cpp

if (!RegUsed) {
       continue;
     }
 
     // MachO's compact unwind format relies on all registers being stored in
     // pairs.
     // FIXME: the usual format is actually better if unwinding isn't needed.
     if (produceCompactUnwindFrame(MF) && !SavedRegs.test(PairedReg)) {
       SavedRegs.set(PairedReg);
+      if (AArch64::GPR64RegClass.contains(PairedReg) &&
+          !RegInfo->isReservedReg(MF, PairedReg))
         ExtraCSSpill = true;
     }
   }
 
   DEBUG(dbgs() << "*** determineCalleeSaves\nUsed CSRs:";
         for (int Reg = SavedRegs.find_first(); Reg != -1;
              Reg = SavedRegs.find_next(Reg))
           dbgs() << ' ' << PrintReg(Reg, RegInfo);
         dbgs() << "\n";);