[PATCH] D120668: [CodeGen] Use AdjustStackOffset for Callee Saved Registers in PEI::calculateFrameObjectOffsets

[Daniel McIntosh via Phabricator via llvm-commits]

DanielMcIntosh-IBM added a comment.

In D120668#3349265 <https://reviews.llvm.org/D120668#3349265>, @arsenm wrote:

> This does look similar at first glance. What's the point of doing this? Why were these paths split in the first place?

They were split when these two loops where first created in rGd31f55c2361068d71f988829d167ce9e24b8f813 <https://reviews.llvm.org/rGd31f55c2361068d71f988829d167ce9e24b8f813>. I'm not sure why they weren't done as 1 loop like I have here.
It seem like the split has already caused issues with changes to one loop not getting mirrored to the other: rG4a57bb5a3b74bdad9b0518009a7d7ac7ca2ac650 <https://reviews.llvm.org/rG4a57bb5a3b74bdad9b0518009a7d7ac7ca2ac650> added a `MaxCSFrameIndex >= MinCSFrameIndex` condition to the else, which was only later mirrored to the other loop by rGea0eec69f16e0f1b00fec413986e4e44f6f627fa <https://reviews.llvm.org/rGea0eec69f16e0f1b00fec413986e4e44f6f627fa>.
Not to mention rG6893926b69088eb65053470802e00081234684a7 <https://reviews.llvm.org/rG6893926b69088eb65053470802e00081234684a7>, which hasn't yet been mirrored, and it's not clear if it should (see my comment).

The goal is to generally tidy things up, and prevent similar issues from happening in the future.



================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:871-873
+      // TODO: should this just be if (MFI.isDeadObjectIndex(FrameIndex))
+      if (!StackGrowsDown && MFI.isDeadObjectIndex(FrameIndex))
         continue;
----------------
This was added by rG6893926b69088eb65053470802e00081234684a7, but only to one of the code paths, so I'm not sure if it should be added to both. @arsenm Any memory of whether there was a good reason to only add it to one half?


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:880-882
+  assert(MaxAlign == MFI.getMaxAlign() &&
+         "MFI.getMaxAlign should already account for all callee-saved "
+         "registers without a fixed stack slot");
----------------
For CSRs, the check/update to MaxAlign by `AdjustStackOffset` should be redundant since rG3b065cdb64b4dcd14c43e8d4e22f2bde680740a7. I'm not sure whether I should do what I have here and assert the expected behaviour, or just pass a dummy value to `AdjustStackOffset` so the compiler can more easily optimize that bit out when it presumably inlines `AdjustStackOffset`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120668