[PATCH] D21424: StackColoring for SafeStack.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/CodeGen/SafeStack.cpp:526
@@ +525,3 @@
+  // Unsafe stack always grows down.
+  SafeStackLayout SSL(StackAlignment, /*GrowsDown*/ true);
+  if (StackGuardSlot) {
----------------
Since the `GrowsDown` parameter is always true, can you remove it?

================
Comment at: lib/CodeGen/SafeStackColoring.cpp:229-233
@@ +228,7 @@
+
+    for (unsigned AllocaNo = 0; AllocaNo < Allocas.size(); ++AllocaNo) {
+      if (Started.test(AllocaNo)) {
+        LiveRanges[AllocaNo].add(Start[AllocaNo], BBEnd);
+      }
+    }
+  }
----------------
You can remove all braces here.

================
Comment at: lib/CodeGen/SafeStackColoring.h:40
@@ +39,3 @@
+public:
+  /// This class represents a set of instructions.
+  struct LiveRange {
----------------
Say which instructions.

================
Comment at: lib/CodeGen/SafeStackColoring.h:41-49
@@ +40,11 @@
+  /// This class represents a set of instructions.
+  struct LiveRange {
+    BitVector bv;
+    void SetMaximum(int size) { bv.resize(size); }
+    void add(unsigned start, unsigned end) { bv.set(start, end); }
+    bool Overlaps(const LiveRange &Other) const {
+      return bv.anyCommon(Other.bv);
+    }
+    void Join(const LiveRange &Other) { bv |= Other.bv; }
+  };
+
----------------
Inconsistent capitalization of member functions.

================
Comment at: lib/CodeGen/SafeStackColoring.h:58
@@ +57,3 @@
+
+  /// Number of interesting instructions.
+  int NumInst;
----------------
Define "interesting"

================
Comment at: lib/CodeGen/SafeStackColoring.h:72
@@ +71,3 @@
+
+  BitVector InterestingAllocas;
+  SmallVector<Instruction *, 8> Markers;
----------------
Define "interesting"

================
Comment at: lib/CodeGen/SafeStackLayout.cpp:69
@@ +68,3 @@
+  DEBUG(dbgs() << "  First candidate: " << Start << " .. " << End << "\n");
+  for (unsigned i = 0; i < Regions.size(); ++i) {
+    StackRegion &R = Regions[i];
----------------
This could be a range-for loop.

================
Comment at: lib/CodeGen/SafeStackLayout.cpp:109
@@ +108,3 @@
+  // Split starting and ending regions if necessary.
+  for (unsigned i = 0; i < Regions.size(); ++i) {
+    StackRegion &R = Regions[i];
----------------
Same here

================
Comment at: lib/CodeGen/SafeStackLayout.cpp:126
@@ +125,3 @@
+  // Update live ranges for all affected regions.
+  for (unsigned i = 0; i < Regions.size(); ++i) {
+    StackRegion &R = Regions[i];
----------------
Same here

================
Comment at: lib/CodeGen/SafeStackLayout.h:18
@@ +17,3 @@
+/// Compute the layout of an unsafe stack frame.
+class SafeStackLayout {
+  unsigned MaxAlignment;
----------------
Is this the right name for this class? The name suggests that it handles layout for the safe stack.


Repository:
  rL LLVM

http://reviews.llvm.org/D21424