[PATCH] D21189: Create subranges for new intervals resulting from live interval splitting

[Matthias Braun via llvm-commits]

MatzeB added a comment.

First: This is a tough problem to solve (nicely), so thanks for pushing!

- I am not happy with extending the core datatype (LiveRange) with an undef list (see below).

- I am also not happy that we just start searching around in the isDefOnEntry() function; Is it realistic to create fake LiveRange Segments (with the Undef VNInfo) and clean them up afterwards?


================
Comment at: include/llvm/CodeGen/LiveInterval.h:199
@@ -197,2 +198,3 @@
     VNInfoList valnos;   // value#'s
+    SlotList undefs;     // explicit "undefs" via <def,read-undef>
 
----------------
I think LiveRange objects are one of the larger memory consumers in codegen. I think we should try hard to not add any members to it. Would it work to create special VNInfo objects during liverange construction (for example set `id = (unsigned)-1` for Undefs)? I think we could restrict those special VNInfos to be only valid during liverange construction and undefined otherwise so we do not need to modify all the functions here to support them, but can restrict ourself to the functions used by LiveRangeCalc.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:510-511
@@ -510,1 +509,4 @@
+void LiveIntervals::shrinkToUses(LiveInterval::SubRange &SR, unsigned Reg) {
+  DEBUG(dbgs() << "Shrink: L" << PrintLaneMask(SR.LaneMask)
+               << ' ' << SR << '\n');
   assert(TargetRegisterInfo::isVirtualRegister(Reg)
----------------
We should override the print() and dump() functions in the SubRange class instead. We can leave that for another patch though.

There are a bunch of slightly unrelated but obvious changes here like this one, the check for readsReg() or the move of the DEBUG print at the end of the function. Would be nice if you could split those into a separate commit (no need to review such obvious changes).

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:527
@@ -524,1 +526,3 @@
       LaneBitmask LaneMask = TRI->getSubRegIndexLaneMask(SubReg);
+      if (MO.isDef() && !MO.isUndef())
+        LaneMask = ~LaneMask & MRI->getMaxLaneMaskForVReg(Reg);
----------------
The MO.isUndef() case cannot happen after you checked MO.readsReg() above.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:586
@@ -580,3 +585,3 @@
   for (unsigned i = 0, e = Indices.size(); i != e; ++i)
-    LRCalc->extend(LR, Indices[i]);
+    LRCalc->extend(LR, Indices[i], 0, AllowUndef);
 }
----------------
Use `/*PhysReg=*/0` to make it clear what the zero is about.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:1557-1558
@@ -1540,2 +1556,4 @@
 void LiveIntervals::removeVRegDefAt(LiveInterval &LI, SlotIndex Pos) {
+  // LI may not have the main range computed yet, but its subranges may
+  // be present.
   VNInfo *VNI = LI.getVNInfoAt(Pos);
----------------
This can only be the case during LiveRangCalc, do we really need to handle this case?

================
Comment at: lib/CodeGen/LiveRangeCalc.cpp:297
@@ -273,1 +296,3 @@
 
+bool LiveRangeCalc::isDefOnEntry(LiveRange &LR, MachineBasicBlock &MBB) {
+  if (DefOnEntry.test(MBB.getNumber()))
----------------
Adding a worklist algorithm adds a dangerous quadratic runtime here (esp. since the results for negative answers are not cached).

================
Comment at: lib/CodeGen/RenameIndependentSubregs.cpp:381-385
@@ -356,2 +380,7 @@
     LIS->constructMainRangeFromSubranges(LI);
+    // A def of a subregister may be a use of other register lanes.
+    // Removing such a def may leave some subranges to extend beyond
+    // actual live ranges.
+    LIS->shrinkToUses(&LI);
+    LI.recalculateSubRangeUndefs();
   }
----------------
This is odd, why do we need to redo shrinkToUses()? Can you give an example or an detailed explanation, the comment isn't clear to me right now.

================
Comment at: lib/CodeGen/SplitKit.cpp:393
@@ +392,3 @@
+  if (!LI.hasSubRanges()) {
+    LI.addSegment(LiveInterval::Segment(Def, Def.getDeadSlot(), VNI));
+    return;
----------------
Maybe we should factor this line out and add a convenience function to LiveRange that complements LiveRange::createDeadDef(), we just need a variant that takes a VNInfo as well.

================
Comment at: lib/CodeGen/SplitKit.cpp:405-406
@@ +404,4 @@
+      if (PV != nullptr && PV->def == Def) {
+        VNInfo *V = S.getNextValue(Def, LIS.getVNInfoAllocator());
+        S.addSegment(LiveInterval::Segment(Def, Def.getDeadSlot(), V));
+      }
----------------
This and the next one looks like LiveRange::createDeadDef() to me.

================
Comment at: lib/CodeGen/SplitKit.cpp:992
@@ -948,3 +991,3 @@
       // Check for a simply defined value that can be blitted directly.
-      ValueForcePair VFP = Values.lookup(std::make_pair(RegIdx, ParentVNI->id));
+      ValueForcePair VFP = Values.lookup({RegIdx, (unsigned)ParentVNI->id});
       if (VNInfo *VNI = VFP.getPointer()) {
----------------
Isn't `ParentVNI->id` already unsigned?


Repository:
  rL LLVM

http://reviews.llvm.org/D21189