[PATCH] D15393: [X86] Order the local stack symbols to improve code size and locality.

[Hans Wennborg via llvm-commits]

hans added a comment.

This looks very interesting.

Maybe update the commit message and/or add to the comments in the file why this is beneficial for code size, since it wasn't obvious for me at least. Do I understand correctly that the idea is that addressing objects closer to %esp generally requires  less code because it can use a tighter addressing mode?


================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:708
@@ -707,1 +707,3 @@
 
+  std::vector<int> ObjectsToAllocate;
+
----------------
Maybe use SmallVector instead?

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:710
@@ -708,2 +709,3 @@
+
   // Then assign frame offsets to stack objects that are not used to spill
   // callee saved registers.
----------------
Since this block of code no longer does the actual assigning, maybe change the comment to "prepare to assign offsets ..." or "gather objects that need stack offsets ..." or something like tthat.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2678
@@ +2677,3 @@
+  // indexing into it, instead of having to search for it every time.
+  std::vector<X86FrameSortingObject> SortingObjects(MFI->getObjectIndexEnd());
+
----------------
Maybe SmallVector?

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2722
@@ +2721,3 @@
+  // For FP, it should be flipped.
+  i = 0;
+  for (auto &Obj : SortingObjects) {
----------------
Declare i here, instead of at the start of the function.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2727
@@ +2726,3 @@
+    else
+      // All invalid items are sorted at the end, so it's safe to stop.
+      break;
----------------
I think breaking early would be more in line with LLVM style.

  if (!Obj.IsValid)
    break;
  ObjectsToAllocate[i++] = ...

================
Comment at: lib/Target/X86/X86FrameLowering.h:205
@@ +204,3 @@
+    bool IsValid;                 // true if we care about this Object.
+    unsigned int ObjectIndex;     // Index of Object into MFI list.
+    unsigned int ObjectSize;      // Size of Object in bytes.
----------------
Just "unsigned" is more common in LLVM code

================
Comment at: lib/Target/X86/X86FrameLowering.h:224
@@ +223,3 @@
+  /// at the end of our list.
+  struct X86FrameSortingAlgorithm {
+    inline bool operator() (const X86FrameSortingObject& a,
----------------
rnk wrote:
> Ditto, sink this to the .cpp file to reduce visibility.
How about calling it X86FrameSortingComparator instead?


http://reviews.llvm.org/D15393