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

[Than McIntosh via llvm-commits]

thanm added inline comments.

================
Comment at: lib/CodeGen/StackColoring.cpp:179
@@ +178,3 @@
+  /// \returns True if inst contains a lifetime start or end
+  bool isLifetimeStartOrEnd(const MachineInstr &MI,
+                            std::vector<int> *slots,
----------------
As far as I know (take this with a grain of salt, since I am an LLVM newbie) the only way to end a lifetime is with a LIFETIME_END marker instruction. If you can think of a scenario where you might have both, then yes, this should definitely be changed. Let me know if you have a real example? 

================
Comment at: lib/CodeGen/StackColoring.cpp:182
@@ -157,1 +181,3 @@
+                            bool *isStart);
+
   /// Construct the LiveIntervals for the slots.
----------------
OK, I will convert to use references.

================
Comment at: lib/CodeGen/StackColoring.cpp:251
@@ -216,1 +250,3 @@
+  }
+}
 
----------------
Yes, all changes to dump methods are refactoring. I will pull them into a separate patch.

================
Comment at: lib/CodeGen/StackColoring.cpp:256
@@ +255,3 @@
+  assert(MI.getOpcode() == TargetOpcode::LIFETIME_START ||
+         MI.getOpcode() == TargetOpcode::LIFETIME_END);
+  const MachineOperand &MO = MI.getOperand(0);
----------------
Will fix.

================
Comment at: lib/CodeGen/StackColoring.cpp:279
@@ +278,3 @@
+      return true;
+    } else {
+      if (!LifetimeStartOnFirstUse || ProtectFromEscapedAllocas) {
----------------
Will fix.

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

================
Comment at: lib/CodeGen/StackColoring.cpp:460-461
@@ -385,10 +459,4 @@
 
-    // 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) {
 
----------------
Will fix.


http://reviews.llvm.org/D18827