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

[Reid Kleckner via llvm-commits]

rnk added a comment.

Our current default stack layout algorithm optimizes for stack frame size without accounting for code size from frame offsets. I'm worried that this algorithm will reorder all the


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2714
@@ +2713,3 @@
+  // info).
+  std::sort(SortingObjects.begin(), SortingObjects.end(),
+            X86FrameSortingAlgorithm());
----------------
hfinkel wrote:
> Based on your comparison function, it looks like this should be stable_sort.
+1

================
Comment at: lib/Target/X86/X86FrameLowering.h:200
@@ +199,3 @@
+  /// Class used by orderFrameObjects to help sort the stack objects.
+  class X86FrameSortingObject {
+  public:
----------------
rnk wrote:
> This can just be a struct. I'd also prefer it if you used default data member initializers instead of a default constructor.
This doesn't need to be in a header, move it to the .cpp file and put it in an anonymous namespace.

================
Comment at: lib/Target/X86/X86FrameLowering.h:200-203
@@ +199,6 @@
+  /// Class used by orderFrameObjects to help sort the stack objects.
+  class X86FrameSortingObject {
+  public:
+    X86FrameSortingObject() : IsValid(false), ObjectIndex(0), ObjectSize(0),
+                              ObjectAlignment(1), ObjectNumUses(0) {}
+    bool IsValid;                 // true if we care about this Object.
----------------
This can just be a struct. I'd also prefer it if you used default data member initializers instead of a default constructor.

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

================
Comment at: lib/Target/X86/X86FrameLowering.h:240
@@ +239,3 @@
+        static_cast<double>(a.ObjectSize);
+      DensityB = static_cast<double>(b.ObjectNumUses) /
+        static_cast<double>(b.ObjectSize);
----------------
hfinkel wrote:
> I'm somewhat afraid of using floating-point comparisons here because you might run into the same problems we had when computing block frequencies/probabilities with floating-point (that you'd get different answers on different systems because of enhanced intermediate precision and other non-strict-IEEE effects). Especially because we have a floating-point equality comparison below.
+1


http://reviews.llvm.org/D15393