ConstraintAnalysis: Sort constraints internally

src/ir/constraint.cpp

@@ -147,13 +147,16 @@ void AndedConstraintSet::approximateAnd(const Constraint& c) {
   }
 
   if (size() < MaxConstraints) {
-    push_back(c);
+    // Insert into the right place, keeping us sorted.
+    insert(std::upper_bound(begin(), end(), c), c);
     return;
   }
 
   // Otherwise, just do not add this one.
   // TODO: We could try to be clever and see if one of the existing ones makes
-  //       more sense to drop.
+  //       more sense to drop. In particular, we should prefer "better" ones
+  //       like > over >= and so forth (sorting more precise ones earlier may be
+  //       useful to implement that).
 }
 
 void AndedConstraintSet::approximateOr(const AndedConstraintSet& other) {

src/ir/constraint.h

@@ -35,6 +35,15 @@ namespace wasm::constraint {
 // A term in a constraint, either a local index or literal value.
 struct Term : public std::variant<Index, Literal> {
   bool operator==(const Term&) const = default;
+  bool operator<(const Term& other) const {
+    if (index() != other.index()) {
+      return index() < other.index();
+    }
+    if (index() == 0) {
+      return std::get<Index>(*this) < std::get<Index>(other);
+    }
+    return std::get<Literal>(*this) < std::get<Literal>(other);
+  }
 };
 
 // A constraint: some operation and some value, like "is equal to 17" or "is
@@ -44,6 +53,12 @@ struct Constraint {
   Term term;
 
   bool operator==(const Constraint&) const = default;
+  bool operator<(const Constraint& other) const {
+    if (op != other.op) {
+      return op < other.op;
+    }
+    return term < other.term;
+  }
 
   Constraint negate() const {
     return Constraint{Abstract::negateRelational(op), term};
@@ -63,6 +78,10 @@ enum Result { True, False, Unknown };
 // the comments below, `x` is used for the thing all the constraints are talking
 // about, which looks like a local, but it could be a global or a struct field
 // or anything else in general.
+//
+// While we are a vector, the order of constraints does not logically matter,
+// and we keep ourselves sorted in a canonical form, so that simple ==, != etc.
+// comparisons work. The canonical order also makes debug printing nicer.
 struct AndedConstraintSet : inplace_vector<Constraint, MaxConstraints> {
   // We could represent a contradiction using two constraints that contradict
   // each other (== 0 && != 0), but for simplicity we mark this explicitly.

src/literal.h

@@ -356,6 +356,7 @@ class Literal {
   // would be equal to itself, if the bits are equal).
   bool operator==(const Literal& other) const;
   bool operator!=(const Literal& other) const;
+  bool operator<(const Literal& other) const;
 
   bool isNaN();
   bool isCanonicalNaN();

src/support/inplace_vector.h

@@ -151,6 +151,9 @@ template<typename T, size_t N> class inplace_vector {
     ConstIterator(const inplace_vector<T, N>* parent, size_t index)
       : wasm::ParentIndexIterator<const inplace_vector<T, N>*, ConstIterator>{
           parent, index} {}
+    ConstIterator(const Iterator& other)
+      : wasm::ParentIndexIterator<const inplace_vector<T, N>*, ConstIterator>{
+          other.parent, other.index} {}
     ConstIterator(const ConstIterator& other) = default;
 
     const T& operator*() const { return (*this->parent)[this->index]; }
@@ -162,6 +165,18 @@ template<typename T, size_t N> class inplace_vector {
   ConstIterator begin() const { return ConstIterator(this, 0); }
   ConstIterator end() const { return ConstIterator(this, size()); }
 
+  Iterator insert(ConstIterator pos, const T& x) {
+    assert(usedFixed < N);
+    assert(pos.index <= usedFixed);
+    size_t index = pos.index;
+    std::move_backward(fixed.begin() + index,
+                       fixed.begin() + usedFixed,
+                       fixed.begin() + usedFixed + 1);
+    fixed[index] = x;
+    usedFixed++;
+    return Iterator(this, index);
+  }
+
   Iterator erase(ConstIterator first, ConstIterator last) {
     assert(first.index <= last.index);
     assert(last.index <= usedFixed);

src/support/parent_index_iterator.h

@@ -55,6 +55,22 @@ template<typename Parent, typename Iterator> struct ParentIndexIterator {
   bool operator!=(const ParentIndexIterator& other) const {
     return !(*this == other);
   }
+  bool operator<(const ParentIndexIterator& other) const {
+    assert(parent == other.parent);
+    return index < other.index;
+  }
+  bool operator<=(const ParentIndexIterator& other) const {
+    assert(parent == other.parent);
+    return index <= other.index;
+  }
+  bool operator>(const ParentIndexIterator& other) const {
+    assert(parent == other.parent);
+    return index > other.index;
+  }
+  bool operator>=(const ParentIndexIterator& other) const {
+    assert(parent == other.parent);
+    return index >= other.index;
+  }
   Iterator& operator++() {
     ++index;
     return self();

src/wasm-interpreter.h

@@ -159,6 +159,9 @@ struct FuncData {
   bool operator==(const FuncData& other) const {
     return name == other.name && self == other.self;
   }
+  bool operator<(const FuncData& other) const {
+    return std::tie(name, self) < std::tie(other.name, other.self);
+  }
 
   Flow doCall(const Literals& arguments) {
     assert(call);

src/wasm/literal.cpp

@@ -520,6 +520,59 @@ bool Literal::operator!=(const Literal& other) const {
   return !(*this == other);
 }
 
+bool Literal::operator<(const Literal& other) const {
+  if (type != other.type) {
+    // This is not deterministic between runs, and also FuncData, below. If this
+    // matters some day, we would need to find a stable way to compute it.
+    return type.getID() < other.type.getID();
+  }
+
+  if (type.isBasic()) {
+    switch (type.getBasic()) {
+      case Type::none:
+        return false;
+      case Type::i32:
+      case Type::f32:
+        return i32 < other.i32;
+      case Type::i64:
+      case Type::f64:
+        return i64 < other.i64;
+      case Type::v128:
+        return memcmp(v128, other.v128, 16) < 0;
+      case Type::unreachable:
+        WASM_UNREACHABLE("invalid literal type");
+    }
+  }
+
+  assert(type.isRef());
+  if (type.isNull()) {
+    // All nulls are equal, and hence not <
+    return false;
+  }
+  if (type.isFunction()) {
+    return *funcData < *other.funcData;
+  }
+  if (type.isData() || type.isString()) {
+    return gcData < other.gcData;
+  }
+  auto heapType = type.getHeapType();
+  assert(heapType.isBasic());
+  if (heapType.isMaybeShared(HeapType::i31)) {
+    return i32 < other.i32;
+  }
+  if (heapType.isMaybeShared(HeapType::ext)) {
+    if (hasExternPayload() != other.hasExternPayload()) {
+      return hasExternPayload() < other.hasExternPayload();
+    }
+    if (hasExternPayload()) {
+      return getExternPayload() < other.getExternPayload();
+    }
+    return internalize() < other.internalize();
+  }
+  assert(heapType.isMaybeShared(HeapType::any));
+  return externalize() < other.externalize();
+}
+
 bool Literal::isNaN() {
   if (type == Type::f32 && std::isnan(getf32())) {
     return true;

test/gtest/inplace_vector.cpp

@@ -84,3 +84,29 @@ TEST_F(InplaceVectorTest, EraseIf) {
   EXPECT_EQ(erased, 3u);
   EXPECT_TRUE(vec.empty());
 }
+
+TEST_F(InplaceVectorTest, Insert) {
+  inplace_vector<int, 6> vec{10, 30, 40};
+
+  // Insert single element in middle (20 at index 1)
+  auto it = vec.insert(vec.begin() + 1, 20);
+  EXPECT_EQ(*it, 20);
+  EXPECT_EQ(vec.size(), 4u);
+  EXPECT_EQ(vec[0], 10);
+  EXPECT_EQ(vec[1], 20);
+  EXPECT_EQ(vec[2], 30);
+  EXPECT_EQ(vec[3], 40);
+
+  // Insert at beginning (5 at index 0)
+  it = vec.insert(vec.begin(), 5);
+  EXPECT_EQ(*it, 5);
+  EXPECT_EQ(vec.size(), 5u);
+  EXPECT_EQ(vec[0], 5);
+  EXPECT_EQ(vec[1], 10);
+
+  // Insert at end (50 at index 5)
+  it = vec.insert(vec.end(), 50);
+  EXPECT_EQ(*it, 50);
+  EXPECT_EQ(vec.size(), 6u);
+  EXPECT_EQ(vec[5], 50);
+}

test/lit/passes/constraint-analysis.wast

@@ -3544,5 +3544,89 @@
       (then)
     )
   )
+
+  ;; CHECK:      (func $iloop (type $8) (param $0 f32)
+  ;; CHECK-NEXT:  (local $1 f32)
+  ;; CHECK-NEXT:  (local.set $0
+  ;; CHECK-NEXT:   (local.get $1)
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT:  (if
+  ;; CHECK-NEXT:   (i32.const 0)
+  ;; CHECK-NEXT:   (then
+  ;; CHECK-NEXT:    (loop
+  ;; CHECK-NEXT:     (local.set $1
+  ;; CHECK-NEXT:      (local.get $0)
+  ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (else
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT:  (loop $label
+  ;; CHECK-NEXT:   (loop
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (br_if $label
+  ;; CHECK-NEXT:    (i32.const 0)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  ;; OPTIN:      (func $iloop (type $8) (param $0 f32)
+  ;; OPTIN-NEXT:  (local $1 f32)
+  ;; OPTIN-NEXT:  (local.set $0
+  ;; OPTIN-NEXT:   (local.get $1)
+  ;; OPTIN-NEXT:  )
+  ;; OPTIN-NEXT:  (if
+  ;; OPTIN-NEXT:   (i32.const 0)
+  ;; OPTIN-NEXT:   (then
+  ;; OPTIN-NEXT:    (loop
+  ;; OPTIN-NEXT:     (local.set $1
+  ;; OPTIN-NEXT:      (local.get $0)
+  ;; OPTIN-NEXT:     )
+  ;; OPTIN-NEXT:    )
+  ;; OPTIN-NEXT:   )
+  ;; OPTIN-NEXT:   (else
+  ;; OPTIN-NEXT:   )
+  ;; OPTIN-NEXT:  )
+  ;; OPTIN-NEXT:  (loop $label
+  ;; OPTIN-NEXT:   (loop
+  ;; OPTIN-NEXT:   )
+  ;; OPTIN-NEXT:   (br_if $label
+  ;; OPTIN-NEXT:    (i32.const 0)
+  ;; OPTIN-NEXT:   )
+  ;; OPTIN-NEXT:  )
+  ;; OPTIN-NEXT: )
+  (func $iloop (param $0 f32)
+    ;; Regression test for an infinite loop: the specific cfg here + the
+    ;; constraints lead to a situation where, if we were not careful, we would
+    ;; think we have an infinite stream of updates in flow(). Specifically, we
+    ;; end up updating a location to a combination of two constraints {A, B} and
+    ;; then end up finding {B, A} in the next cycle, and then alternate those
+    ;; two forever. This is fixed by sorting the constraints.
+    ;;
+    ;; (There is nothing to optimize here, we just should not hang or error.)
+    (local $1 f32)
+    (local.set $0
+      (local.get $1)
+    )
+    (if
+      (i32.const 0)
+      (then
+        (loop
+          (local.set $1
+            (local.get $0)
+          )
+        )
+      )
+      (else
+      )
+    )
+    (loop $label
+      (loop
+      )
+      (br_if $label
+        (i32.const 0)
+      )
+    )
+  )
 )