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

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2660-2662
@@ -2659,1 +2659,5 @@
 
+namespace {
+  // Struct used by orderFrameObjects to help sort the stack objects.
+  struct X86FrameSortingObject {
+    bool IsValid{false};         // true if we care about this Object.
----------------
Please don't indent in namespaces.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2663-2667
@@ +2662,7 @@
+  struct X86FrameSortingObject {
+    bool IsValid{false};         // true if we care about this Object.
+    unsigned ObjectIndex{0};     // Index of Object into MFI list.
+    unsigned ObjectSize{0};      // Size of Object in bytes.
+    unsigned ObjectAlignment{1}; // Alignment of Object in bytes.
+    unsigned ObjectNumUses{0};   // Object static number of uses.
+  };
----------------
Please use `=` for initializers.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2683-2686
@@ +2682,6 @@
+  // at the end of our list.
+  struct X86FrameSortingComparator {
+    inline bool operator() (const X86FrameSortingObject& a,
+                            const X86FrameSortingObject& b)
+    {
+      int64_t DensityAScaled, DensityBScaled;
----------------
Please clang -format this.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2684-2685
@@ +2683,4 @@
+  struct X86FrameSortingComparator {
+    inline bool operator() (const X86FrameSortingObject& a,
+                            const X86FrameSortingObject& b)
+    {
----------------
Pointers and references lean right.  Variables should be capitalized.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2707-2710
@@ +2706,6 @@
+      // arithmetic.
+      DensityAScaled = static_cast<int64_t>(a.ObjectNumUses) *
+        static_cast<int64_t>(b.ObjectSize);
+      DensityBScaled = static_cast<int64_t>(b.ObjectNumUses) *
+        static_cast<int64_t>(a.ObjectSize);
+
----------------
Why signed?

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2756-2757
@@ +2755,4 @@
+    if (ObjectSize == 0)
+      // Variable size. Just use 4.
+      SortingObjects[Obj].ObjectSize = 4;
+    else      
----------------
Why?  Why not 1?

================
Comment at: lib/Target/X86/X86FrameLowering.h:142
@@ +141,3 @@
+  void orderFrameObjects(const MachineFunction &MF,
+                         SmallVectorImpl<int> &objectsToAllocate) const override;
+
----------------
Please capitalize variable names.


http://reviews.llvm.org/D15393