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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
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) {
----------------
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.


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

https://reviews.llvm.org/D89239