[PATCH] D16831: [regalloc][WinEH] Do not mark intervals as not spillable if they contain a regmask

[Andy Kaylor via llvm-commits]

andrew.w.kaylor added inline comments.

================
Comment at: include/llvm/CodeGen/LiveInterval.h:550
@@ +549,3 @@
+    // the interval may be required.
+    bool containsRegMask(ArrayRef<SlotIndex> RegMaskSlots) const;
+
----------------
MatzeB wrote:
> I'd rather call this function something like liveAtIndexes() and also talk about indexes instead of regmasks in the comment. The comments in the header and implementation should just talk about SlotIndexes. The details about regmasks and spilling can be mentioned where this function is used.
OK.  I'll make that change.

================
Comment at: lib/CodeGen/LiveInterval.cpp:757-762
@@ +756,8 @@
+    return false;
+  // Otherwise, see if any regmask is contained by any of our segments.
+  for (const Segment &S : segments) {
+    // Find the first regmask slot at or after this segment start.
+    while (*SlotI < S.start)
+      if (++SlotI == SlotE)
+        return false;
+    // If the segment contains this slot, we have our answer.
----------------
MatzeB wrote:
> This does not need to be a quadratic algorithm as the regmaskslots and segments are both sorted!
> 
> I'd initialize an iterator with segments.begin(), loop over the RegMaskSlots and use advanceTo() to check each index.
I don't think this is quadratic the way I've implemented it.  The inner while loop just advances the slot iterator from wherever it was (at the start of the current for loop iteration) to the first slot that is at or after the start of the current segment.  This will never visit any slot from RegMaskSlots or any segment more than once.

I think that what you are suggesting would accomplish the same thing, just switching the hierarchy of the two loops and using advanceTo in place of the inner loop.  I'm happy to do it that way if you think it reads better.  I admit that the way I have written this code it does look quadratic.


Repository:
  rL LLVM

http://reviews.llvm.org/D16831