[PATCH] D31583: StackColoring: smarter check for slot overlap

[Than McIntosh via Phabricator via llvm-commits]

thanm requested changes to this revision.
thanm added a comment.
This revision now requires changes to proceed.

I pulled your patch and did a bootstrap build with it, no issues. Overall this seems like an improvement over the initial patch. Please see inline comments.



================
Comment at: lib/CodeGen/StackColoring.cpp:186
+// From our rules, we see that *out-of-scope* slots are never *in-use*,
+// and from (L7) we see that "non-conservative" slots remain non-*in-use*
+// until their address is taken. Therefore, we can approximate slot activity
----------------
This is kind of a nit, but if I am reading this correctly, your use of "non-conservative" here refers to a slot that is not degenerate, which is not talked about until down on line 342. Maybe it would make sense to put in a forward reference?


================
Comment at: lib/CodeGen/StackColoring.cpp:833
             Starts[Slot] = ThisIndex;
-        } else {
-          if (!Finishes[Slot].isValid() || Finishes[Slot] < ThisIndex)
-            Finishes[Slot] = ThisIndex;
+        } else if (!IsStart && Starts[Slot].isValid()) {
+          VNInfo *VNI = Intervals[Slot]->getValNumInfo(0);
----------------
I see "if (!IsStart && ..." -- seems like the "!IsStart" is redundant at this point, no?


================
Comment at: lib/CodeGen/StackColoring.cpp:1188
+        // Implementation Notes section for an explanation.
+        if (!First->isLiveAtIndexes(SecondS) &&
+            !Second->isLiveAtIndexes(FirstS)) {
----------------
A couple of observations:

First, relative to your first patch I think I can make more more sense out of what's going on. I can also see where you're getting the improvement, since doing the conflict check based on starts is inherently more precise. 

Second, there is a concern that in a large function with many packets overlapped together you could get into quadratic compile time behavior (since in the worst case the interval has O(N) items and the LiveStarts vector has O(N) items). If you haven't seen any issues in practice, though, I suppose it's probably not worrying too much about.



https://reviews.llvm.org/D31583