diff --git a/src/ir/constraint.cpp b/src/ir/constraint.cpp index 6332f8a5224..a55a77d6889 100644 --- a/src/ir/constraint.cpp +++ b/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) { diff --git a/src/ir/constraint.h b/src/ir/constraint.h index 1c91e0d1632..e3be8305268 100644 --- a/src/ir/constraint.h +++ b/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 { 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(*this) < std::get(other); + } + return std::get(*this) < std::get(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 { // We could represent a contradiction using two constraints that contradict // each other (== 0 && != 0), but for simplicity we mark this explicitly. diff --git a/src/literal.h b/src/literal.h index e982dae68aa..6f8415aeeb4 100644 --- a/src/literal.h +++ b/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(); diff --git a/src/support/inplace_vector.h b/src/support/inplace_vector.h index 09b8d3abf15..da365ce7fb3 100644 --- a/src/support/inplace_vector.h +++ b/src/support/inplace_vector.h @@ -151,6 +151,9 @@ template class inplace_vector { ConstIterator(const inplace_vector* parent, size_t index) : wasm::ParentIndexIterator*, ConstIterator>{ parent, index} {} + ConstIterator(const Iterator& other) + : wasm::ParentIndexIterator*, ConstIterator>{ + other.parent, other.index} {} ConstIterator(const ConstIterator& other) = default; const T& operator*() const { return (*this->parent)[this->index]; } @@ -162,6 +165,18 @@ template 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); diff --git a/src/support/parent_index_iterator.h b/src/support/parent_index_iterator.h index e19aacfdcfc..8495356eed7 100644 --- a/src/support/parent_index_iterator.h +++ b/src/support/parent_index_iterator.h @@ -55,6 +55,22 @@ template 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(); diff --git a/src/wasm-interpreter.h b/src/wasm-interpreter.h index 120fc78bea5..c5c1a33ef17 100644 --- a/src/wasm-interpreter.h +++ b/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); diff --git a/src/wasm/literal.cpp b/src/wasm/literal.cpp index d20921bccbf..a44a1c6e906 100644 --- a/src/wasm/literal.cpp +++ b/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; diff --git a/test/gtest/inplace_vector.cpp b/test/gtest/inplace_vector.cpp index 0480bc9f79f..0f477f0f624 100644 --- a/test/gtest/inplace_vector.cpp +++ b/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 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); +} diff --git a/test/lit/passes/constraint-analysis.wast b/test/lit/passes/constraint-analysis.wast index 207a8e65993..a94e6d50d04 100644 --- a/test/lit/passes/constraint-analysis.wast +++ b/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) + ) + ) + ) )