[PATCH] D9067: LiveIntervalAnalysis: Support moving of subregister defs in handleMove

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Matthias,

You mentioned that you found and fixed several regressions. Yet, this patch does not have any test cases added. Am I correctly assuming this is because the related test cases are already in the repository?
If not, could you please add them.

You also have a couple of missing period at the end of the comments. I’ve highlighted the first few ones. Please double check.

The patch mostly LGTM, though the comments would benefits from more examples (like you did with “ where I = [xi…”). Please double check the potential overlapping issue I pointed out.

Thanks,
-Quentin


================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1016
@@ +1015,3 @@
+  ///    the range that contains NewIdx.
+  ///    (Happens when reordering independent writes to a subregister)
+  ///
----------------
Period.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1033
@@ +1032,3 @@
+      // happens when moving over an unrelated subregister def.
+      // Check if we need to extend the destination interval and return
+      LiveRange::iterator Next = std::next(I);
----------------
The phrasing does not make sense.
I am guessing we want something like:
If <…>, check <…>.
This happens <….>.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1033
@@ +1032,3 @@
+      // happens when moving over an unrelated subregister def.
+      // Check if we need to extend the destination interval and return
+      LiveRange::iterator Next = std::next(I);
----------------
qcolombet wrote:
> The phrasing does not make sense.
> I am guessing we want something like:
> If <…>, check <…>.
> This happens <….>.
Period.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1041
@@ +1040,3 @@
+        LiveInterval::iterator Prev = std::prev(NewI);
+        Prev->end = NewIdx.getRegSlot();
+        return;
----------------
At this point, NewI must be NewIdx, right?
So, this will create an overlap from base index to reg slot, right?

What I am missing?

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1077
@@ -1058,2 +1076,3 @@
     // 3. Dead def at OldIdx: I->end = OldIdx.getDeadSlot().
+    // 6. Live def at OldIdx moving accross several others values
     // In either case, it is possible that there is an existing def at NewIdx.
----------------
Period.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1089
@@ +1088,3 @@
+        // [x0,x1)...[xi,xi+1)[xi+1,xi+2)...
+        // There is no gap between I and its predecessor, merge them
+        DefVNI = I->valno;
----------------
Period.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1096
@@ +1095,3 @@
+        // [x0,x1)...[xi,xi+1)[xi+2,xi+3)...
+        // The value is live in in I
+        LiveRange::iterator Next = std::next(I);
----------------
Period.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1099
@@ +1098,3 @@
+        // We merge I and its successor. As we're dealing with subreg
+        // reordering, there is always a successor to I in the same BB
+        DefVNI = Next->valno;
----------------
Period.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1099
@@ +1098,3 @@
+        // We merge I and its successor. As we're dealing with subreg
+        // reordering, there is always a successor to I in the same BB
+        DefVNI = Next->valno;
----------------
qcolombet wrote:
> Period.
Assert on Next.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1175
@@ +1174,3 @@
+  ///    contains NewIdx.
+  ///    (Happens when reordering independent partial write to a register)
+  ///
----------------
Period.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1230
@@ +1229,3 @@
+      // Case 6:
+      // OldIdx is not a dead def and NewIdx is before predecessor start
+      LiveInterval::iterator NewI = LR.find(NewIdx.getRegSlot(true));
----------------
Period.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1233
@@ +1232,3 @@
+      const SlotIndex SplitPos = DefVNI->def;
+      // We merge I and I predecessor's
+      LiveRange::iterator Prev = std::prev(I);
----------------
Period.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1240
@@ +1239,3 @@
+      std::copy_backward(NewI, Prev, I);
+      // NewI is now considered undef
+      LiveRange::iterator Next = std::next(NewI);
----------------
Period.


Repository:
  rL LLVM

http://reviews.llvm.org/D9067