[PATCH] D18827: Rework/enhance stack coloring data flow analysis.

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Than,

Have you run clang-format?
Some of the formatting looks strange to me.

I haven’t looked into the algo yet.

Cheers,
-Quentin


================
Comment at: lib/CodeGen/StackColoring.cpp:176
@@ +175,3 @@
+  /// accordingly.
+  /// \returns True if inst contains a lifetime start or end
+  bool isLifetimeStartOrEnd(const MachineInstr &MI,
----------------
Can’t we theoretically have an interesting lifetime start and end on the same instruction.
The API does not allow to model that. What happens in that case?

================
Comment at: lib/CodeGen/StackColoring.cpp:179
@@ -156,1 +178,3 @@
+                            std::vector<int> *slots,
+                            bool *isStart);
 
----------------
Having look at the implementation, this method does not support nullptr as input argument so please, use references.

================
Comment at: lib/CodeGen/StackColoring.cpp:249
@@ -216,1 +248,3 @@
+  }
+}
 
----------------
All the dump methods are refactoring right?
If so, please commit them separately.

================
Comment at: lib/CodeGen/StackColoring.cpp:254
@@ +253,3 @@
+  assert(MI.getOpcode() == TargetOpcode::LIFETIME_START ||
+         MI.getOpcode() == TargetOpcode::LIFETIME_END);
+  const MachineOperand &MO = MI.getOperand(0);
----------------
Add a message in assert.

================
Comment at: lib/CodeGen/StackColoring.cpp:277
@@ +276,3 @@
+      return true;
+    } else {
+      if (!LifetimeStartOnFirstUse || ProtectFromEscapedAllocas) {
----------------
Get rid of the else.
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

================
Comment at: lib/CodeGen/StackColoring.cpp:354
@@ -246,2 +353,3 @@
 
+    std::vector<int> slots;
     for (MachineInstr &MI : *MBB) {
----------------
SmallVector?

================
Comment at: lib/CodeGen/StackColoring.cpp:458
@@ -385,9 +457,3 @@
 
-    // Create the interval for the basic blocks with lifetime markers in them.
-    for (const MachineInstr *MI : Markers) {
-      if (MI->getParent() != &MBB)
-        continue;
-
-      assert((MI->getOpcode() == TargetOpcode::LIFETIME_START ||
-              MI->getOpcode() == TargetOpcode::LIFETIME_END) &&
-             "Invalid Lifetime marker");
+    // Create the interval for the basic blocks containing lifetime begin/end
+    for (const MachineInstr &MI : MBB) {
----------------
Period.


http://reviews.llvm.org/D18827