Bug 1772282 - (part 2 of 3) Replace js/src/ds/SplayTree.h with an AvlTree.h and change all of SM's uses accordingly. r=jandem.

This changes all 3 of SM's uses of js/src/ds/SplayTree.h to use
js/src/ds/AvlTree.h.  The new interface is almost identical to the old one, so
the changes are mostly trivial:

(0) js/src/jit/JitcodeMap.h: two comments referencing unknown "trees" have
been amended.

(1) js/src/ds/MemoryProtectionExceptionHandler.cpp: this uses a tree to record
memory ranges that are protected (?).  The only change is of the type of the
tree.

(2) BacktrackingAllocator.h: a minor use, to record ranges containing calls
(`BacktrackingAllocator::callRanges`).  Also just a change of type.  It would
be possible to use the AVL trees to merge the partially-redundant fields
`::callRanges` and `::callRangesList`, but that is beyond the scope of this
patch.

(3) BacktrackingAllocator.h: the main use: changing `LiveRangeSet` to use an
AvlTree.  This is also just a renaming of the type.

(3, more) struct `PrintLiveRange` has been removed.  It was a workaround for
the fact that the splay trees had no iteration facility.  Its use, in
BacktrackingAllocator::dumpAllocations, has been replaced by an AVL iterator.

(3, more) Note that this change causes the allocator to produce different
allocations.  This is because the allocator depends on the actual tree layout,
specifically which node is closest to the root when more than one node matches
a query, and that's different for the two tree implementations.

This behaviour manifests in BacktrackingAllocator::tryAllocateRegister, where
register-use trees are queried:

  if (!rAlias.allocations.contains(range, &existing)) {
    continue;
  }

This asks "does the tree contain a range that overlaps `range`?; if yes,
return it in `existing`". If more than one range in the tree overlaps `range`,
which one is written to `existing` is arbitrary.  The code goes on to decide
whether it's OK to evict the bundle containing existing based (in part) on
`existing`s spill weight.

This could be seen as a bug in the logic in that if `existing` has a low spill
weight then it may choose to evict `existing`s bundle, even though some other
range -- that wasn't returned -- has a higher spill weight. Hence it could
incorrectly decide to evict a bundle that has a higher spill weight than the
bundle for which allocation is attempted.

The above analysis may be a misinterpretation of the logic.  Multiple attempts
to "fix" it were made, without success.  In any case the resulting
allocations are marginally better.  See
https://bugzilla.mozilla.org/show_bug.cgi?id=1772282#c2

Differential Revision: https://phabricator.services.mozilla.com/D148247

js/src/ds/MemoryProtectionExceptionHandler.cpp

@@ -26,7 +26,7 @@
 #  include <android/log.h>
 #endif
 
-#include "ds/SplayTree.h"
+#include "ds/AvlTree.h"
 
 #include "threading/LockGuard.h"
 #include "threading/Thread.h"
@@ -69,7 +69,7 @@ class ProtectedRegionTree {
 
   Mutex lock MOZ_UNANNOTATED;
   LifoAlloc alloc;
-  SplayTree<Region, Region> tree;
+  AvlTree<Region, Region> tree;
 
  public:
   ProtectedRegionTree()

js/src/jit/BacktrackingAllocator.cpp

@@ -3628,21 +3628,6 @@ void BacktrackingAllocator::dumpLiveRangesByBundle(const char* who) {
   }
 }
 
-struct BacktrackingAllocator::PrintLiveRange {
-  bool& first_;
-
-  explicit PrintLiveRange(bool& first) : first_(first) {}
-
-  void operator()(const LiveRange* range) {
-    if (first_) {
-      first_ = false;
-    } else {
-      fprintf(stderr, " /");
-    }
-    fprintf(stderr, " %s", range->toString().get());
-  }
-};
-
 void BacktrackingAllocator::dumpAllocations() {
   JitSpew(JitSpew_RegAlloc, "Allocations:");
 
@@ -3656,7 +3641,16 @@ void BacktrackingAllocator::dumpAllocations() {
       JitSpewHeader(JitSpew_RegAlloc);
       JitSpewCont(JitSpew_RegAlloc, "  %s:", AnyRegister::FromCode(i).name());
       bool first = true;
-      registers[i].allocations.forEach(PrintLiveRange(first));
+      LiveRangeSet::Iter lrIter(&registers[i].allocations);
+      while (lrIter.hasMore()) {
+        LiveRange* range = lrIter.next();
+        if (first) {
+          first = false;
+        } else {
+          fprintf(stderr, " /");
+        }
+        fprintf(stderr, " %s", range->toString().get());
+      }
       JitSpewCont(JitSpew_RegAlloc, "\n");
     }
   }

js/src/jit/BacktrackingAllocator.h

@@ -11,8 +11,8 @@
 #include "mozilla/Atomics.h"
 #include "mozilla/Attributes.h"
 
+#include "ds/AvlTree.h"
 #include "ds/PriorityQueue.h"
-#include "ds/SplayTree.h"
 #include "jit/RegisterAllocator.h"
 #include "jit/StackSlotAllocator.h"
 
@@ -558,7 +558,9 @@ class BacktrackingAllocator : protected RegisterAllocator {
 
   PriorityQueue<QueueItem, QueueItem, 0, SystemAllocPolicy> allocationQueue;
 
-  using LiveRangeSet = SplayTree<LiveRange*, LiveRange>;
+  // This is a set of LiveRanges.  They must be non-overlapping.  Attempts to
+  // add an overlapping range will cause AvlTree::insert to MOZ_CRASH().
+  using LiveRangeSet = AvlTree<LiveRange*, LiveRange>;
 
   // Each physical register is associated with the set of ranges over which
   // that register is currently allocated.
@@ -595,7 +597,7 @@ class BacktrackingAllocator : protected RegisterAllocator {
   // Ranges where all registers must be spilled due to call instructions.
   using CallRangeList = InlineList<CallRange>;
   CallRangeList callRangesList;
-  SplayTree<CallRange*, CallRange> callRanges;
+  AvlTree<CallRange*, CallRange> callRanges;
 
   // Information about an allocated stack slot.
   struct SpillSlot : public TempObject,
@@ -773,7 +775,6 @@ class BacktrackingAllocator : protected RegisterAllocator {
 #ifdef JS_JITSPEW
   void dumpLiveRangesByVReg(const char* who);
   void dumpLiveRangesByBundle(const char* who);
-  struct PrintLiveRange;
   void dumpAllocations();
 #endif
 

js/src/jit/JitcodeMap.h

@@ -372,8 +372,7 @@ class JitcodeGlobalEntry {
   };
 
   // QueryEntry is never stored in the table, just used for queries
-  // where an instance of JitcodeGlobalEntry is required to do tree
-  // lookups.
+  // where an instance of JitcodeGlobalEntry is required to do lookups.
   struct QueryEntry : public BaseEntry {
     void init(void* addr) { BaseEntry::init(Query, nullptr, addr, addr); }
     uint8_t* addr() const {
@@ -403,8 +402,8 @@ class JitcodeGlobalEntry {
     // Dummy entries.
     DummyEntry dummy_;
 
-    // When doing queries on the SplayTree for particular addresses,
-    // the query addresses are representd using a QueryEntry.
+    // When doing queries for particular addresses, the query addresses are
+    // represented using a QueryEntry.
     QueryEntry query_;
   };