Revert of [turbofan] Splinter into one range. (patchset #2 id:80001 of...

Revert of [turbofan] Splinter into one range. (patchset #2 id:80001 of https://codereview.chromium.org/1391023007/ )

Reason for revert:
Weird endless loop in TopLevelLiveRange::Merge() due to always splitting first and not making progress. See comments, unfortunately no useable repro.

Original issue's description:
> [turbofan] Splinter into one range.
>
> Before this CL, we created one live range per successive set of
> deferred blocks. For scenarios with many such blocks, this creates
> an upfront pressure for the register allocator to deal with many ranges.
> Linear sorts ranges, which is a super-linear operation.
>
> The change places all deferred intervals into one range, meaning that,
> at most, there will be twice as many live ranges as the original set. In
> pathological cases (benchmarks/Compile/slow_nbody1.js), this change
> halves the compilation time. We see some improvements elsewhere,
> notably SQLite at ~4-5%.
>
> We may be able to avoid the subsequent merge. Its cost is the
> additional ranges it may need to create. The sole reason for the merge
> phase is to provide an unchanged view of the world to the subsequent
> phases. With the at-most-one splinter model, we may be able to teach
> the other phases about splintering - should we find perf hindrances
> due to merging.
>
> Committed: https://crrev.com/efdcd20267870276c5824f1ccf4e171ac378f7ae
> Cr-Commit-Position: refs/heads/master@{#31224}

TBR=jarin@chromium.org,mtrofin@google.com,mtrofin@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Review URL: https://codereview.chromium.org/1403163003

Cr-Commit-Position: refs/heads/master@{#31300}

src/compiler/live-range-separator.cc

@@ -78,14 +78,12 @@ void CreateSplinter(TopLevelLiveRange *range, RegisterAllocationData *data,
     if (range->MayRequireSpillRange()) {
       data->CreateSpillRangeForLiveRange(range);
     }
-    if (range->splinter() == nullptr) {
-      TopLevelLiveRange *splinter = data->NextLiveRange(range->machine_type());
-      DCHECK_NULL(data->live_ranges()[splinter->vreg()]);
-      data->live_ranges()[splinter->vreg()] = splinter;
-      range->SetSplinter(splinter);
-    }
+    TopLevelLiveRange *result = data->NextLiveRange(range->machine_type());
+    DCHECK_NULL(data->live_ranges()[result->vreg()]);
+    data->live_ranges()[result->vreg()] = result;
+
     Zone *zone = data->allocation_zone();
-    range->Splinter(start, end, zone);
+    range->Splinter(start, end, result, zone);
   }
 }
 

src/compiler/register-allocator.cc

@@ -698,8 +698,7 @@ TopLevelLiveRange::TopLevelLiveRange(int vreg, MachineType machine_type)
       spilled_in_deferred_blocks_(false),
       spill_start_index_(kMaxInt),
       last_child_(this),
-      last_pos_(nullptr),
-      splinter_(nullptr) {
+      last_insertion_point_(this) {
   bits_ |= SpillTypeField::encode(SpillType::kNoSpillType);
 }
 
@@ -846,12 +845,12 @@ AllocatedOperand TopLevelLiveRange::GetSpillRangeOperand() const {
 
 
 void TopLevelLiveRange::Splinter(LifetimePosition start, LifetimePosition end,
-                                 Zone* zone) {
+                                 TopLevelLiveRange* result, Zone* zone) {
   DCHECK(start != Start() || end != End());
   DCHECK(start < end);
 
-  TopLevelLiveRange splinter_temp(-1, machine_type());
-  UsePosition* last_in_splinter = nullptr;
+  result->set_spill_type(spill_type());
+
   if (start <= Start()) {
     // TODO(mtrofin): here, the TopLevel part is in the deferred range, so we
     // may want to continue processing the splinter. However, if the value is
@@ -860,21 +859,21 @@ void TopLevelLiveRange::Splinter(LifetimePosition start, LifetimePosition end,
     // should check this, however, this may not be the place, because we don't
     // have access to the instruction sequence.
     DCHECK(end < End());
-    DetachAt(end, &splinter_temp, zone);
+    DetachAt(end, result, zone);
     next_ = nullptr;
   } else if (end >= End()) {
     DCHECK(start > Start());
-    DetachAt(start, &splinter_temp, zone);
+    DetachAt(start, result, zone);
     next_ = nullptr;
   } else {
     DCHECK(start < End() && Start() < end);
 
     const int kInvalidId = std::numeric_limits<int>::max();
 
-    UsePosition* last = DetachAt(start, &splinter_temp, zone);
+    UsePosition* last = DetachAt(start, result, zone);
 
     LiveRange end_part(kInvalidId, this->machine_type(), nullptr);
-    last_in_splinter = splinter_temp.DetachAt(end, &end_part, zone);
+    result->DetachAt(end, &end_part, zone);
 
     next_ = end_part.next_;
     last_interval_->set_next(end_part.first_interval_);
@@ -891,39 +890,20 @@ void TopLevelLiveRange::Splinter(LifetimePosition start, LifetimePosition end,
       if (last != nullptr) last->set_next(end_part.first_pos_);
     }
   }
+  result->next_ = nullptr;
+  result->top_level_ = result;
 
-  if (splinter()->IsEmpty()) {
-    splinter()->first_interval_ = splinter_temp.first_interval_;
-    splinter()->last_interval_ = splinter_temp.last_interval_;
-  } else {
-    splinter()->last_interval_->set_next(splinter_temp.first_interval_);
-    splinter()->last_interval_ = splinter_temp.last_interval_;
-  }
-  if (splinter()->first_pos() == nullptr) {
-    splinter()->first_pos_ = splinter_temp.first_pos_;
-  } else {
-    splinter()->last_pos_->set_next(splinter_temp.first_pos_);
-  }
-  if (last_in_splinter != nullptr) {
-    splinter()->last_pos_ = last_in_splinter;
-  } else {
-    if (splinter()->first_pos() != nullptr &&
-        splinter()->last_pos_ == nullptr) {
-      splinter()->last_pos_ = splinter()->first_pos();
-      for (UsePosition* pos = splinter()->first_pos(); pos != nullptr;
-           pos = pos->next()) {
-        splinter()->last_pos_ = pos;
-      }
-    }
-  }
-#if DEBUG
-  Verify();
-  splinter()->Verify();
-#endif
+  result->SetSplinteredFrom(this);
+  // Ensure the result's relative ID is unique within the IDs used for this
+  // virtual register's children and splinters.
+  result->relative_id_ = GetNextChildId();
 }
 
 
 void TopLevelLiveRange::SetSplinteredFrom(TopLevelLiveRange* splinter_parent) {
+  // The splinter parent is always the original "Top".
+  DCHECK(splinter_parent->Start() < Start());
+
   splintered_from_ = splinter_parent;
   if (!HasSpillOperand() && splinter_parent->spill_range_ != nullptr) {
     SetSpillRange(splinter_parent->spill_range_);
@@ -948,55 +928,43 @@ void TopLevelLiveRange::Merge(TopLevelLiveRange* other, Zone* zone) {
   DCHECK(Start() < other->Start());
   DCHECK(other->splintered_from() == this);
 
-  LiveRange* first = this;
-  LiveRange* second = other;
-  DCHECK(first->Start() < second->Start());
-  while (first != nullptr && second != nullptr) {
-    DCHECK(first != second);
-    // Make sure the ranges are in order each time we iterate.
-    if (second->Start() < first->Start()) {
-      LiveRange* tmp = second;
-      second = first;
-      first = tmp;
-      continue;
-    }
+  LiveRange* last_other = other->last_child();
+  LiveRange* last_me = last_child();
 
-    if (first->End() <= second->Start()) {
-      if (first->next() == nullptr ||
-          first->next()->Start() > second->Start()) {
-        // First is in order before second.
-        LiveRange* temp = first->next();
-        first->next_ = second;
-        first = temp;
-      } else {
-        // First is in order before its successor (or second), so advance first.
-        first = first->next();
-      }
-      continue;
-    }
+  // Simple case: we just append at the end.
+  if (last_me->End() <= other->Start()) return last_me->AppendAsChild(other);
 
-    DCHECK(first->Start() < second->Start());
-    // If first and second intersect, split first.
-    if (first->Start() < second->End() && second->Start() < first->End()) {
-      LiveRange* temp = first->SplitAt(second->Start(), zone);
+  DCHECK(last_me->End() > last_other->End());
 
-      temp->set_spilled(first->spilled());
-      if (!temp->spilled())
-        temp->set_assigned_register(first->assigned_register());
+  // In the more general case, we need to find the ranges between which to
+  // insert.
+  if (other->Start() < last_insertion_point_->Start()) {
+    last_insertion_point_ = this;
+  }
 
-      first->next_ = second;
-      first = temp;
-      continue;
-    }
-    DCHECK(first->End() <= second->Start());
+  for (; last_insertion_point_->next() != nullptr &&
+         last_insertion_point_->next()->Start() <= other->Start();
+       last_insertion_point_ = last_insertion_point_->next()) {
   }
 
-  TopLevel()->UpdateParentForAllChildren(TopLevel());
-  TopLevel()->UpdateSpillRangePostMerge(other);
+  // When we splintered the original range, we reconstituted the original range
+  // into one range without children, but with discontinuities. To merge the
+  // splinter back in, we need to split the range - or a child obtained after
+  // register allocation splitting.
+  LiveRange* after = last_insertion_point_->next();
+  if (last_insertion_point_->End() > other->Start()) {
+    LiveRange* new_after = last_insertion_point_->SplitAt(other->Start(), zone);
+    new_after->set_spilled(last_insertion_point_->spilled());
+    if (!new_after->spilled())
+      new_after->set_assigned_register(
+          last_insertion_point_->assigned_register());
+    after = new_after;
+  }
 
-#if DEBUG
-  Verify();
-#endif
+  last_other->next_ = after;
+  last_insertion_point_->next_ = other;
+  other->UpdateParentForAllChildren(TopLevel());
+  TopLevel()->UpdateSpillRangePostMerge(other);
 }
 
 

src/compiler/register-allocator.h

@@ -493,7 +493,8 @@ class TopLevelLiveRange final : public LiveRange {
   // result.
   // The current range is pointed to as "splintered_from". No parent/child
   // relationship is established between this and result.
-  void Splinter(LifetimePosition start, LifetimePosition end, Zone* zone);
+  void Splinter(LifetimePosition start, LifetimePosition end,
+                TopLevelLiveRange* result, Zone* zone);
 
   // Assuming other was splintered from this range, embeds other and its
   // children as part of the children sequence of this range.
@@ -537,6 +538,7 @@ class TopLevelLiveRange final : public LiveRange {
     spill_start_index_ = Min(start, spill_start_index_);
   }
 
+  void SetSplinteredFrom(TopLevelLiveRange* splinter_parent);
   void CommitSpillsAtDefinition(InstructionSequence* sequence,
                                 const InstructionOperand& operand,
                                 bool might_be_duplicated);
@@ -576,20 +578,8 @@ class TopLevelLiveRange final : public LiveRange {
   }
   void set_last_child(LiveRange* range) { last_child_ = range; }
   LiveRange* last_child() const { return last_child_; }
-  TopLevelLiveRange* splinter() const { return splinter_; }
-  void SetSplinter(TopLevelLiveRange* splinter) {
-    DCHECK_NULL(splinter_);
-    DCHECK_NOT_NULL(splinter);
-
-    splinter_ = splinter;
-    splinter->relative_id_ = GetNextChildId();
-    splinter->set_spill_type(spill_type());
-    splinter->SetSplinteredFrom(this);
-  }
 
  private:
-  void SetSplinteredFrom(TopLevelLiveRange* splinter_parent);
-
   typedef BitField<bool, 1, 1> HasSlotUseField;
   typedef BitField<bool, 2, 1> IsPhiField;
   typedef BitField<bool, 3, 1> IsNonLoopPhiField;
@@ -609,8 +599,7 @@ class TopLevelLiveRange final : public LiveRange {
   bool spilled_in_deferred_blocks_;
   int spill_start_index_;
   LiveRange* last_child_;
-  UsePosition* last_pos_;
-  TopLevelLiveRange* splinter_;
+  LiveRange* last_insertion_point_;
 
   DISALLOW_COPY_AND_ASSIGN(TopLevelLiveRange);
 };

test/unittests/compiler/live-range-unittest.cc

@@ -32,14 +32,11 @@ class LiveRangeUnitTest : public TestWithZone {
 
   TopLevelLiveRange* Splinter(TopLevelLiveRange* top, int start, int end,
                               int new_id = 0) {
-    if (top->splinter() == nullptr) {
-      TopLevelLiveRange* ret =
-          new (zone()) TopLevelLiveRange(new_id, MachineType::kRepTagged);
-      top->SetSplinter(ret);
-    }
+    TopLevelLiveRange* ret =
+        new (zone()) TopLevelLiveRange(new_id, MachineType::kRepTagged);
     top->Splinter(LifetimePosition::FromInt(start),
-                  LifetimePosition::FromInt(end), zone());
-    return top->splinter();
+                  LifetimePosition::FromInt(end), ret, zone());
+    return ret;
   }
 
   // Ranges first and second match structurally.
@@ -380,25 +377,6 @@ TEST_F(LiveRangeUnitTest, SplinterMultipleIntervalsRight) {
 }
 
 
-TEST_F(LiveRangeUnitTest, SplinterMergeMultipleTimes) {
-  TopLevelLiveRange* range =
-      TestRangeBuilder(zone()).Add(0, 3).Add(5, 10).Add(12, 16).Build();
-  Splinter(range, 4, 6);
-  Splinter(range, 8, 14);
-  TopLevelLiveRange* splinter = range->splinter();
-  EXPECT_EQ(nullptr, range->next());
-  EXPECT_EQ(nullptr, splinter->next());
-  EXPECT_EQ(range, splinter->splintered_from());
-
-  TopLevelLiveRange* expected_source =
-      TestRangeBuilder(zone()).Add(0, 3).Add(6, 8).Add(14, 16).Build();
-  TopLevelLiveRange* expected_splinter =
-      TestRangeBuilder(zone()).Add(5, 6).Add(8, 10).Add(12, 14).Build();
-  EXPECT_TRUE(RangesMatch(expected_source, range));
-  EXPECT_TRUE(RangesMatch(expected_splinter, splinter));
-}
-
-
 TEST_F(LiveRangeUnitTest, MergeMultipleIntervalsRight) {
   TopLevelLiveRange* original =
       TestRangeBuilder(zone()).Add(0, 3).Add(5, 8).Build();
@@ -438,9 +416,8 @@ TEST_F(LiveRangeUnitTest, IDGeneration) {
 
   TopLevelLiveRange* splinter =
       new (zone()) TopLevelLiveRange(101, MachineType::kRepTagged);
-  vreg->SetSplinter(splinter);
   vreg->Splinter(LifetimePosition::FromInt(4), LifetimePosition::FromInt(12),
-                 zone());
+                 splinter, zone());
 
   EXPECT_EQ(101, splinter->vreg());
   EXPECT_EQ(1, splinter->relative_id());