[PATCH] D115690: [SystemZ] Implement orderFrameObjects().

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added reviewers: eugenis, zansari.
jonpa added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:196
+    // Pack if possible smaller objects before the last (big) one inside U12.
+    for (++CurrentPos;
+         TopUsed > 0 && CurrentOffset < TopUsed - 1 && CurrentPos != IE;
----------------
The effect of the packing of smaller objects below the bigger one is relatively small. Commenting this out I see:

```
lay            :                54005                54970     +965
la             :               534200               533379     -821
l              :               178549               178356     -193
ly             :                 3574                 3767     +193
sty            :                 3528                 3663     +135
st             :               122536               122401     -135
ley            :                  369                  410      +41
lde            :                68636                68595      -41
...
OPCDIFFS: 139 
```




================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:218
+#ifndef NDEBUG
+  void recordExpectedRanges(const MachineFrameInfo &MFI,
+                            SystemZMachineFunctionInfo *ZFI) {
----------------
These checks were useful during experiments, but could perhaps be reduced if the patch is considered more trustworthy...




================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:259
+  void dumpResult(StringRef F) {
+    dbgs() << "++ Reordered SystemZ frame objects for function " << F << " ++\n";
+    dbgs() << "Estim offs    FI Align      Size  Density   D12Freq  "
----------------
This dumping looks nice to me, but is not really necessary...


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:392
+#ifndef NDEBUG
+  // Sanity check the result.
+  assert(Idx == ObjectsToAllocate.size() && "Objects sorting havoc!");
----------------
Perhaps this is also no longer needed...?


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:711
     // Create 2 for the case where both addresses in an MVC are
-    // out of range.
+    // out of range. TODO: possible to improve (see orderFrameObjects)?
     RS->addScavengingFrameIndex(MFFrame.CreateStackObject(8, Align(8), false));
----------------
Maybe it would be possible to first sort the objects and then decide if the slot(s) are needed. I guess however this would typically only be for big functions where that wouldn't matter much..?


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

https://reviews.llvm.org/D115690