From 6ee583fae0c84d5ff4996844e1177501655de190 Mon Sep 17 00:00:00 2001 From: Philip Hall Date: Wed, 22 Jan 2025 10:59:42 +0000 Subject: [PATCH] MLBEDSW-10299: Size types and allocator fixes for ordered_map - Normalise the sized types used by ordered_map to present an interface more consistent with the standard library. - Set initial allocation to zero such that declaring an empty map allocates no storage, and add tests for the same. Storage is now allocated on first-use. - Fix potential range issue with initial hashtable size being greater than the chosen indexer. - Fix issue where it was not possible to resize up to the maximum indexer limit. Signed-off-by: Philip Hall Change-Id: I12742431808d73625ac6bcdbd7b701b52f763834 --- ethosu/regor/common/ordered_map.hpp | 105 ++++++++++++-------- ethosu/regor/compiler/tensor_properties.hpp | 7 +- ethosu/regor/test/test_ordered_map.cpp | 67 +++++++++---- ethosu/regor/tflite/tflite_reader.cpp | 4 +- ethosu/regor/tflite/tflite_writer.cpp | 6 +- 5 files changed, 122 insertions(+), 67 deletions(-) diff --git a/ethosu/regor/common/ordered_map.hpp b/ethosu/regor/common/ordered_map.hpp index d1dea42d..cf0e86a7 100644 --- a/ethosu/regor/common/ordered_map.hpp +++ b/ethosu/regor/common/ordered_map.hpp @@ -90,6 +90,8 @@ protected: static constexpr INDEXER HASH_FREE = INDEXER(-2); static constexpr INDEXER HASH_END = INDEXER(-1); static constexpr INDEXER NODE_UNLINKED = INDEXER(-1); + static constexpr size_t DEFAULT_INITIAL_SIZE = 3; + static constexpr size_t MAX_INDEX_CAPACITY = size_t(std::numeric_limits::max() - 2); // Node stores two arrays in one allocation. // - Items array, mapping key->untyped storage @@ -117,19 +119,22 @@ protected: }; std::unique_ptr _items; // Bulk node store - int _capacity = 0; // Total allocated capacity - int _itemCount = 0; // Number of inserted/valid items + INDEXER _capacity = 0; // Total allocated capacity + INDEXER _itemCount = 0; // Number of inserted/valid items INDEXER _tableSize = 0; // Hash table size INDEXER _orderBegin = NODE_UNLINKED; // First item in insertion order INDEXER _orderLast = NODE_UNLINKED; // Last item in insertion order INDEXER _allocWatermark = 0; // Location of last overflow allocation public: - ordered_map(int initialCapacity = 3) { resize(initialCapacity); } + ordered_map(size_t initialCapacity = 0) + { + if ( initialCapacity ) resize(initialCapacity); + } ordered_map(std::initializer_list> init) { - resize(int(init.size())); + resize(init.size()); for ( auto &pair : init ) { emplace(pair.first, pair.second); @@ -138,7 +143,7 @@ public: ordered_map(const std::pair *pairs, size_t count) { - resize(int(count)); + resize(count); while ( count-- ) { emplace(pairs->first, pairs->second); @@ -204,10 +209,12 @@ public: ~ordered_map() { clear(); } public: - int size() const { return _itemCount; } + size_t size() const { return size_t(_itemCount); } bool empty() const { return _itemCount == 0; } + size_t capacity() const { return size_t(_capacity); } bool contains(const KEY &key) const { + if ( empty() ) return false; int index = table_index_of(key); return find_node(index, key) != nullptr; } @@ -241,6 +248,7 @@ public: template VALUE &emplace(const KEY &key, Args &&...args) { + if ( !_tableSize ) resize(DEFAULT_INITIAL_SIZE); int index = table_index_of(key); Node *node = find_node(index, key); if ( node == nullptr ) @@ -259,6 +267,7 @@ public: void insert(const KEY &key, const VALUE &value) { + if ( !_tableSize ) resize(DEFAULT_INITIAL_SIZE); int index = table_index_of(key); Node *node = find_node(index, key); if ( node == nullptr ) @@ -274,6 +283,7 @@ public: void insert(const KEY &key, VALUE &&value) { + if ( !_tableSize ) resize(DEFAULT_INITIAL_SIZE); int index = table_index_of(key); Node *node = find_node(index, key); if ( node == nullptr ) @@ -302,6 +312,7 @@ public: void reinsert(const KEY &key, const VALUE &value) { + if ( !_tableSize ) resize(DEFAULT_INITIAL_SIZE); int index = table_index_of(key); Node *node = find_node(index, key); if ( node == nullptr ) @@ -330,6 +341,7 @@ public: void reinsert(const KEY &key, VALUE &&value) { + if ( !_tableSize ) resize(DEFAULT_INITIAL_SIZE); int index = table_index_of(key); Node *node = find_node(index, key); if ( node == nullptr ) @@ -378,6 +390,7 @@ public: VALUE &operator[](const KEY &key) { + if ( !_tableSize ) resize(DEFAULT_INITIAL_SIZE); int index = table_index_of(key); Node *node = find_node(index, key); if ( node == nullptr ) @@ -398,6 +411,7 @@ public: const VALUE &at(const KEY &key) const { + if ( !_tableSize ) throw std::out_of_range("not initialised"); int index = table_index_of(key); const Node *node = find_node(index, key); if ( node == nullptr ) throw std::out_of_range("missing key"); @@ -408,6 +422,7 @@ public: bool try_get(const KEY &key, VALUE &value) const { + if ( empty() ) return false; int index = table_index_of(key); const Node *node = find_node(index, key); if ( node != nullptr ) @@ -420,6 +435,7 @@ public: const VALUE *try_ref(const KEY &key) const { + if ( empty() ) return nullptr; int index = table_index_of(key); const Node *node = find_node(index, key); if ( node != nullptr ) @@ -671,6 +687,7 @@ public: int erase(const KEY &key) { + if ( !_tableSize ) return 0; int index = table_index_of(key); int prevIndex = -1; // local sentinel - not the indexer value Node *node = find_node(index, key, &prevIndex); @@ -716,6 +733,7 @@ public: bool key_of(const VALUE &value, KEY &key) const { + if ( !_tableSize ) return false; int order = _orderBegin; while ( order != NODE_UNLINKED ) { @@ -731,10 +749,8 @@ public: } private: - ordered_map(this_class_t &other, int capacity) + ordered_map(this_class_t &other, size_t capacity) { - assert(capacity < std::numeric_limits::max() - 2); // Capacity exceeds indexer - resize(capacity); // Duplicate in source's insertion order @@ -751,11 +767,16 @@ private: } } - void resize(int capacity) + void resize(size_t capacity) { - int newTableSize = hashtable_size(capacity); - assert(capacity < std::numeric_limits::max() - 2); // Capacity exceeds indexer - if ( capacity <= _capacity ) return; + // Resize capacity limited to chosen indexer + capacity = std::min(capacity, MAX_INDEX_CAPACITY); + INDEXER newTableSize = INDEXER(hashtable_size(capacity)); + if ( capacity <= size_t(_capacity) ) + { + assert(_capacity != MAX_INDEX_CAPACITY && "Reached Indexer Limit"); + return; + } // Same hash table size, just move old items into the new item storage if ( !_items || _tableSize == newTableSize ) @@ -775,9 +796,9 @@ private: } } _items = std::move(newItems); - _capacity = capacity; + _capacity = INDEXER(capacity); _tableSize = INDEXER(newTableSize); - _allocWatermark = INDEXER(capacity - 1); + _allocWatermark = PURE_HASH_CHAINS ? _tableSize : 0; } else // Rehash by stealing the internals of another map { @@ -905,6 +926,7 @@ private: void update_node_order(Node *node) { const auto index = INDEXER(node - _items.get()); + assert(size_t(index) < MAX_INDEX_CAPACITY); if ( node->order_prev == NODE_UNLINKED ) { _orderBegin = index; @@ -936,11 +958,13 @@ private: { node->order_next = position->order_next; node->order_prev = INDEXER(position - _items.get()); + assert(size_t(node->order_prev) < MAX_INDEX_CAPACITY); } else { node->order_next = INDEXER(position - _items.get()); node->order_prev = position->order_prev; + assert(size_t(node->order_next) < MAX_INDEX_CAPACITY); } update_node_order(node); @@ -963,6 +987,8 @@ private: // Point them at each other instead. const auto firstIndex = INDEXER(first - _items.get()); const auto secondIndex = INDEXER(second - _items.get()); + assert(size_t(firstIndex) < MAX_INDEX_CAPACITY); + assert(size_t(secondIndex) < MAX_INDEX_CAPACITY); if ( firstPrev == secondIndex ) { @@ -984,6 +1010,7 @@ private: { // Initial index must be within the hashtable assert(index >= 0 && index < _tableSize); + assert(_items); // Node is unallocated if ( _items[index].hash_next == HASH_FREE ) @@ -1039,53 +1066,53 @@ private: void operator()(TYPE &dst, TYPE &src) const { std::memcpy(&dst, &src, sizeof(TYPE)); } }; - int table_index_of(const KEY &key, int tableSize) const { return int(HASH()(key, size_t(tableSize))); } + int table_index_of(const KEY &key, INDEXER tableSize) const + { + assert(tableSize); + return int(HASH()(key, size_t(tableSize))); + } int table_index_of(const KEY &key) const { return table_index_of(key, _tableSize); } int find_free_index() { - const int minAlloc = PURE_HASH_CHAINS ? _tableSize : 0; + const INDEXER MIN_ALLOC = PURE_HASH_CHAINS ? _tableSize : 0; - // Crude search for a free slot (start at the watermark) - for ( int i = _allocWatermark; i >= minAlloc; --i ) - { - if ( _items[i].hash_next == HASH_FREE ) - { - _allocWatermark = INDEXER(i - 1); - return i; - } - } - for ( int i = _capacity - 1; i > _allocWatermark; --i ) + // Search for a free slot, starting before the watermark. + INDEXER i = _allocWatermark; + do { + i--; + if ( (i < MIN_ALLOC) || (i >= _capacity) ) i = _capacity - 1; if ( _items[i].hash_next == HASH_FREE ) { - _allocWatermark = INDEXER(i - 1); + _allocWatermark = i; // Watermark should be a valid (but used) slot. + assert(_allocWatermark >= MIN_ALLOC && _allocWatermark < _capacity); return i; } - } + } while ( i != _allocWatermark ); + return -1; // Error, no free slots } - int hashtable_size(int &capacity) + size_t hashtable_size(size_t &capacity) { // Prime table is tuned for rehashing while the capacity is small. Whereas // table sizes for larger capacities are sparser, when rehashing is expensive. // Note: Not all starting capacities give the same resize pattern! // clang-format off - static constexpr int primes[] = { 3, 7, 11, 13, 17, 19, 23, 31, 41, 67, 97, 131, - 197, 257, 509, 1021, 2039, 4093, 8191, 16381 }; + static constexpr size_t primes[] = { 3, 7, 11, 13, 17, 19, 23, 31, 41, 67, 97, 131, + 197, 257, 509, 1021, 2039, 4093, 8191, 16381 }; // clang-format on - assert(capacity < std::numeric_limits::max() - 2); // Capacity exceeds indexer - capacity = std::max(capacity, 2); + capacity = std::max(capacity, 2); // Estimate to expect ~1 collision per element - int estimatedTableSize = (capacity + 1) / 2; - int tableSize = 2; + size_t estimatedTableSize = (capacity + 1) / 2; + size_t tableSize = 2; // Choose a conservative prime hashtable size for small capacities // (stops rehashing after final table size has been seen) - for ( int i : primes ) + for ( size_t i : primes ) { - if ( i > estimatedTableSize ) + if ( i > estimatedTableSize || tableSize >= MAX_INDEX_CAPACITY ) { break; } @@ -1093,7 +1120,7 @@ private: } // Ensure capacity includes space other than just the table - capacity = std::max(capacity, tableSize * 2); + capacity = std::min(std::max(capacity, tableSize * 2), MAX_INDEX_CAPACITY); assert(tableSize != 0); return tableSize; } diff --git a/ethosu/regor/compiler/tensor_properties.hpp b/ethosu/regor/compiler/tensor_properties.hpp index 8af81654..2ab44e5c 100644 --- a/ethosu/regor/compiler/tensor_properties.hpp +++ b/ethosu/regor/compiler/tensor_properties.hpp @@ -1,5 +1,5 @@ // -// SPDX-FileCopyrightText: Copyright 2023-2024 Arm Limited and/or its affiliates +// SPDX-FileCopyrightText: Copyright 2023-2025 Arm Limited and/or its affiliates // // SPDX-License-Identifier: Apache-2.0 // @@ -75,9 +75,10 @@ constexpr inline bool IsIFM(TensorUsage usage) return (usage & TensorUsage::TypeMask) == TensorUsage::IFM; } -constexpr inline TensorUsage MakeTensorUsage(TensorUsage type, int index) +template +constexpr inline TensorUsage MakeTensorUsage(TensorUsage type, NUMERIC index) { - return TensorUsage(uint32_t(type) | (index << 8)); + return TensorUsage(uint32_t(type) | (uint32_t(index) << 8)); } constexpr inline int GetUsageIndex(TensorUsage usage) diff --git a/ethosu/regor/test/test_ordered_map.cpp b/ethosu/regor/test/test_ordered_map.cpp index 1ac3e372..0ef4d142 100644 --- a/ethosu/regor/test/test_ordered_map.cpp +++ b/ethosu/regor/test/test_ordered_map.cpp @@ -37,9 +37,9 @@ bool operator==(const std::array &lhs, const std::array &rhs) template static void CheckInsertionOrder(const ordered_map &uut, const std::unordered_map &ref, const KEY expected_keys[]) { - REQUIRE(uut.size() == int(ref.size())); + REQUIRE(uut.size() == ref.size()); - int i = 0; + size_t i = 0; for ( auto pair : uut.pairs() ) { REQUIRE(i < uut.size()); @@ -81,7 +81,7 @@ TEST_CASE("ordered_map: Retains insertion order") // 3: Using a combination of all three for ( int test = 0; test < 4; ++test ) { - for ( int capacity = 1; capacity <= 256; capacity *= 2 ) + for ( size_t capacity = 1; capacity <= 256; capacity *= 2 ) { ordered_map uut{capacity}; std::unordered_map ref; @@ -112,7 +112,7 @@ TEST_CASE("ordered_map: Retains insertion order") } } - REQUIRE(uut.size() == int(ref.size())); + REQUIRE(uut.size() == ref.size()); CheckInsertionOrder(uut, ref, u"An orde_mapwilyst"); @@ -176,7 +176,7 @@ TEST_CASE("ordered_map: Reinserts") using Key = char16_t; using Val = std::array; - for ( int capacity = 1; capacity <= 256; capacity *= 2 ) + for ( size_t capacity = 1; capacity <= 256; capacity *= 2 ) { ordered_map uut{capacity}; std::unordered_map ref; @@ -239,7 +239,7 @@ TEST_CASE("ordered_map: Reorders") using Key = char16_t; using Val = int; - for ( int capacity = 1; capacity <= 256; capacity *= 2 ) + for ( size_t capacity = 1; capacity <= 256; capacity *= 2 ) { ordered_map uut{capacity}; std::unordered_map ref; @@ -307,7 +307,7 @@ TEST_CASE("ordered_map: Swaps") using Key = char16_t; using Val = int; - for ( int capacity = 1; capacity <= 256; capacity *= 2 ) + for ( size_t capacity = 1; capacity <= 256; capacity *= 2 ) { ordered_map uut{capacity}; std::unordered_map ref; @@ -411,7 +411,7 @@ TEST_CASE("ordered_map: Resizes (move assignable)") static_assert(std::is_move_assignable_v); - for ( int capacity = 1; capacity <= 256; capacity *= 2 ) + for ( size_t capacity = 1; capacity <= 256; capacity *= 2 ) { ordered_map uut{capacity}; @@ -446,7 +446,7 @@ TEST_CASE("ordered_map: Resizes (trivially copyable)") static_assert(!std::is_move_assignable_v); static_assert(std::is_trivially_copyable_v); - for ( int capacity = 1; capacity <= 256; capacity *= 2 ) + for ( size_t capacity = 1; capacity <= 256; capacity *= 2 ) { ordered_map uut{capacity}; @@ -482,7 +482,7 @@ TEST_CASE("ordered_map: Resizes (not move assignable, not trivially copyable)") static_assert(!std::is_move_assignable_v); static_assert(!std::is_trivially_copyable_v); - for ( int capacity = 1; capacity <= 256; capacity *= 2 ) + for ( size_t capacity = 1; capacity <= 256; capacity *= 2 ) { ordered_map uut{capacity}; @@ -599,7 +599,7 @@ TEST_CASE("ordered_map: Behaves like a map") using Key = unsigned char; using Val = int; - for ( int capacity = 1; capacity <= 256; capacity *= 2 ) + for ( size_t capacity = 1; capacity <= 256; capacity *= 2 ) { ordered_map uut{capacity}; std::unordered_map ref; @@ -643,7 +643,7 @@ TEST_CASE("ordered_map: Behaves like a map") { // Capacity case MapLikeOp::Size: - CHECK(uut.size() == int(ref.size())); + CHECK(uut.size() == ref.size()); break; case MapLikeOp::Empty: CHECK(uut.empty() == ref.empty()); @@ -778,7 +778,7 @@ TEST_CASE("ordered_map: Behaves like a map") case MapLikeOp::CheckAll: case MapLikeOp::Clear: // Recently added elements may have never been checked, so check before clearing default: - CHECK(uut.size() == int(ref.size())); + CHECK(uut.size() == ref.size()); CHECK(uut.empty() == ref.empty()); for ( auto &pair : ref ) { @@ -798,7 +798,7 @@ TEST_CASE("ordered_map: Behaves like a map") { // Additional checks after every operation in debug mode to end the test closer to the point of failure. // Disabled by default in case the act of reading the uut after every write masks a bug. - REQUIRE(uut.size() == int(ref.size())); + REQUIRE(uut.size() == ref.size()); REQUIRE(uut.empty() == ref.empty()); for ( auto &pair : ref ) { @@ -892,7 +892,7 @@ TEST_CASE("ordered_map: Behaves like a vector") using Key = size_t; using Val = Key; - for ( int capacity = 1; capacity <= 256; capacity *= 2 ) + for ( size_t capacity = 1; capacity <= 256; capacity *= 2 ) { ordered_map uut{capacity}; std::vector ref; @@ -959,7 +959,7 @@ TEST_CASE("ordered_map: Behaves like a vector") { // Capacity case VectorLikeOp::Size: - CHECK(uut.size() == int(ref.size())); + CHECK(uut.size() == ref.size()); break; case VectorLikeOp::Empty: CHECK(uut.empty() == ref.empty()); @@ -1074,7 +1074,7 @@ TEST_CASE("ordered_map: Behaves like a vector") uut[keyval] = keyval; break; case VectorLikeOp::PushBackSeveral: // Add several elements in a row - elements_to_add = int(urandom_range(1, capacity)); + elements_to_add = int(urandom_range(1, unsigned(capacity))); break; // Iterate through every element @@ -1082,7 +1082,7 @@ TEST_CASE("ordered_map: Behaves like a vector") case VectorLikeOp::Clear: // Recently added elements may have never been checked, so check before // clearing { - CHECK(uut.size() == int(ref.size())); + CHECK(uut.size() == ref.size()); CHECK(uut.empty() == ref.empty()); auto actual_it = uut.begin(); for ( auto expect : ref ) @@ -1115,7 +1115,7 @@ TEST_CASE("ordered_map: Behaves like a vector") { // Additional checks after every operation in debug mode to end the test closer to the point of failure. // Disabled by default in case the act of reading the uut after every write masks a bug. - REQUIRE(uut.size() == int(ref.size())); + REQUIRE(uut.size() == ref.size()); REQUIRE(uut.empty() == ref.empty()); auto actual_it = uut.begin(); for ( auto expect : ref ) @@ -1191,7 +1191,7 @@ static const std::pair defaultValues[] = { TEST_CASE("ordered_map: initialised construction") { ordered_map map(defaultValues, std::size(defaultValues)); - REQUIRE(map.size() == int(std::size(defaultValues))); + REQUIRE(map.size() == std::size(defaultValues)); auto pos = map.begin(); for ( size_t i = 0; i < std::size(defaultValues); i++ ) { @@ -1253,3 +1253,30 @@ TEST_CASE("ordered_map: key_of inverse query") REQUIRE(map.key_of("text1", temp)); REQUIRE(temp == 111); } + +TEST_CASE("ordered_map: zero initial capacity") +{ + ordered_map map; + REQUIRE(map.capacity() == 0); + map[1] = "hello"; + REQUIRE(map.capacity() > 0); +} + +TEST_CASE("ordered_map: unsigned indexer") +{ + ordered_map, uint8_t> map(127); + + for ( int i = 0; i < 253; i++ ) + { + map.emplace(i, i); + } + + bool allPresent = true; + for ( int i = 0; i < 253; i++ ) + { + allPresent = allPresent && (map.at(i) == i); + } + + REQUIRE(map.capacity() == 253); // Internal sizing behaviour should clamp + REQUIRE(allPresent); +} diff --git a/ethosu/regor/tflite/tflite_reader.cpp b/ethosu/regor/tflite/tflite_reader.cpp index d1d1e189..ba362cc6 100644 --- a/ethosu/regor/tflite/tflite_reader.cpp +++ b/ethosu/regor/tflite/tflite_reader.cpp @@ -654,14 +654,14 @@ void TfLiteReader::ParseOperatorOptions(const std::shared_ptr &operat case tflite::BuiltinOptions::SplitOptions: { int num_splits = GetBuiltinOptions(tflite_operator)->num_splits(); - if ( num_splits != operation->Outputs().size() ) operation->SetPassthroughOp(); + if ( size_t(num_splits) != operation->Outputs().size() ) operation->SetPassthroughOp(); } break; case tflite::BuiltinOptions::SplitVOptions: { int num_splits = GetBuiltinOptions(tflite_operator)->num_splits(); - if ( num_splits != operation->Outputs().size() ) operation->SetPassthroughOp(); + if ( size_t(num_splits) != operation->Outputs().size() ) operation->SetPassthroughOp(); } break; diff --git a/ethosu/regor/tflite/tflite_writer.cpp b/ethosu/regor/tflite/tflite_writer.cpp index db70f242..f814f99f 100644 --- a/ethosu/regor/tflite/tflite_writer.cpp +++ b/ethosu/regor/tflite/tflite_writer.cpp @@ -1,5 +1,5 @@ // -// SPDX-FileCopyrightText: Copyright 2021-2024 Arm Limited and/or its affiliates +// SPDX-FileCopyrightText: Copyright 2021-2025 Arm Limited and/or its affiliates // // SPDX-License-Identifier: Apache-2.0 // @@ -609,13 +609,13 @@ flatbuffers::Offset TfLiteWriter::SerialiseOptions(const Operation *operat case tflite::BuiltinOptions::SplitOptions: { - offset = tflite::CreateSplitOptions(_flatbuffer, operation->Outputs().size()).Union(); + offset = tflite::CreateSplitOptions(_flatbuffer, int(operation->Outputs().size())).Union(); } break; case tflite::BuiltinOptions::SplitVOptions: { - offset = tflite::CreateSplitVOptions(_flatbuffer, operation->Outputs().size()).Union(); + offset = tflite::CreateSplitVOptions(_flatbuffer, int(operation->Outputs().size())).Union(); } break; -- GitLab