[PATCH] D89239: [RISCV][PrologEpilogInserter] "Float" emergency spill slots to avoid making them immediately unreachable from the stack pointer

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

LGTM, thanks @rogfer01!



================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:1083
+
+    // If we have increased the offset to fulfill the alignment constrants,
+    // then the scavenging spill slots may become harder to reach from the
----------------
nit: constraints


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:1092
+      int64_t Delta = Offset - OffsetBeforeAlignment;
+      for (SmallVectorImpl<int>::iterator I = SFIs.begin(), IE = SFIs.end();
+           I != IE; ++I) {
----------------
rogfer01 wrote:
> sdesmalen wrote:
> > rogfer01 wrote:
> > > sdesmalen wrote:
> > > > Is this approach the same as setting the alignment of the last SFI to StackAlign?
> > > Not sure I understood you comment.
> > > 
> > > This removes the additional offset that was introduced in line 1081 for all the SFIs, not only the last one.
> > You're right, these are all allocated individually, so changing the alignment of the last SFI would indeed only affect the position of that one.
> > 
> > I guess I was thinking along the lines to what is done for the Local stack allocation block. i.e. allocate the SFIs together as one block (in this case aligned to the stack alignment), and then adjust the individual offsets for each of the SFIs? That would place the logic for this in one place. Now it first assigns offsets for the slots, then calculates stack alignment, and then fixes up the offsets based on the stack alignment. Is that something you have considered?
> Hi Sander, thanks for the suggestion. I hadn't considered it. I understand you suggest to do the adjustment earlier, in the loop in line 1053.
> 
> However I took a look and so far it does not seem an obvious thing to me. It seems I still have to take into account things like `!TFI.targetHandlesStackFrameRounding()`  and `MFI.adjustsStack() || MFI.hasVarSizedObjects()`, etc. Also `MaxAlign` is passed by reference to `AdjustStackoffset` and may change during the loop. I am afraid it would make the code a messier. I could do that after the loop (before 1056) but I still have to copy some of the checks and calculations that happen in the body of 1058.
My thought was more to create a new single object that comprises all SFIs, that is either allocated early or late, possibly adjusted for stack alignment. And then have one loop that allocates the individual SFIs 'within' that object, by setting offsets based on the encompassing object's start address.  But thinking this through, it probably wouldn't be much of an improvement to the current mechanism, as it still requires allocating an object and adjusting it's offset.

Apologies if I have stalled the patch with this suggestion.


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

https://reviews.llvm.org/D89239