diff --git a/projects/hipdnn/docs/AddingNewOperations.md b/projects/hipdnn/docs/AddingNewOperations.md index b98de457959c..cf10af36ae4f 100644 --- a/projects/hipdnn/docs/AddingNewOperations.md +++ b/projects/hipdnn/docs/AddingNewOperations.md @@ -190,6 +190,10 @@ the right one. Both mistakes compile: A name is not evidence: `PointwiseAttributes.axis_tensor_uid` is an axis index despite the suffix, and is correctly unannotated. +The schema must contain exactly one domain-shaped vector: a root vector of tables whose element +carries one integer `cache_uid_key`. The generator refuses to emit when a second candidate +exists, because ordinals would have no defined answer. + ### Adding a field 1. Annotate it, or deliberately leave it unannotated. diff --git a/projects/hipdnn/docs/Environment.md b/projects/hipdnn/docs/Environment.md index c21bcaf860e6..0376259679be 100644 --- a/projects/hipdnn/docs/Environment.md +++ b/projects/hipdnn/docs/Environment.md @@ -210,7 +210,7 @@ export HIPDNN_FORCE_BENCHMARKING=0 ``` **Notes:** -- With benchmarking on, the first `execute()` of a plan samples every knob-filtered candidate kernel before caching the winner for the plan's life, so it is slower than subsequent calls. +- With benchmarking on and no matching ranking cached, the first `execute()` of a plan samples every knob-filtered candidate kernel, so it is slower than subsequent calls. The ingestor memoizes the ranking per (graph content, device); it survives the plan and persists across processes when disk caching is enabled. - The variable is process-wide, with no per-provider or per-engine granularity: a leaked value from one test or shell changes an unrelated run. - `HIPDNN_FORCE_BENCHMARKING=0` also defeats `Graph::autotune()` in EXHAUSTIVE mode, which otherwise sets `global.benchmarking=1` on its priming plans. - Sampling executes each candidate against the buffers you passed in, so benchmarking assumes idempotent execution or separate input and output buffers -- the same assumption `autotune()` documents. A graph whose output tensor is also one of its inputs is recomputed in place once per sample. The winner runs last, so the final contents are correct, but the buffer is written many times before that. diff --git a/projects/hipdnn/docs/PluginDevelopment.md b/projects/hipdnn/docs/PluginDevelopment.md index b54d16191b9a..546b4f6577b2 100644 --- a/projects/hipdnn/docs/PluginDevelopment.md +++ b/projects/hipdnn/docs/PluginDevelopment.md @@ -412,11 +412,11 @@ public: #### Implementing `global.benchmarking` -The generic kernel ingestor engine (`hipdnn_plugin_sdk::ingestor`) implements the global benchmarking knob: setting `global.benchmarking=1` makes it build one candidate plan per knob-filtered catalog entry, time each on the device on first `execute()`, and reuse the fastest for the plan's remaining life. Per-field knobs plus `add_engine_sweep()` remain the way to sweep one metadata dimension and reach every kernel a declared field distinguishes; the two mechanisms are complementary. A provider adopting the ingestor needs no benchmarking code of its own: carry one `hipdnn_plugin_sdk::ingestor::IngestorSettings ingestorSettings` member on its `TSettings` type and a handle exposing `hipStream_t getStream() const`, and `GenericPlanBuilder` handles advertisement, knob reading, the `HIPDNN_FORCE_BENCHMARKING` override, and the composite plan. +The generic kernel ingestor engine (`hipdnn_plugin_sdk::ingestor`) implements the global benchmarking knob: setting `global.benchmarking=1` makes it build one candidate plan per knob-filtered catalog entry, time each on the device on first `execute()`, and memoize the ranking per (graph content, device). The ranking survives the plan that measured it, and persists across processes when disk caching is enabled. Per-field knobs plus `add_engine_sweep()` remain the way to sweep one metadata dimension and reach every kernel a declared field distinguishes; the two mechanisms are complementary. A provider adopting the ingestor needs no benchmarking code of its own: carry one `hipdnn_plugin_sdk::ingestor::IngestorSettings ingestorSettings` member on its `TSettings` type and a handle exposing `hipStream_t getStream() const`, and `GenericPlanBuilder` handles advertisement, knob reading, the `HIPDNN_FORCE_BENCHMARKING` override, and the composite plan. A handle used with the ingestor **must** expose `hipStream_t getStream() const`: benchmarking's default timer brackets each candidate with HIP events on that stream. Both `GenericPlanBuilder` and `BenchmarkPlan` `static_assert` the requirement, so a handle without it fails to compile with a message naming it rather than failing at plan-build time. The requirement sits on the ingestor templates, not on `validateHandleType()`, so a provider that never instantiates them is unaffected. `BenchmarkPlan` accepts an optional `Timer` in place of that default; it exists so tests can prove candidate selection without a device, and is not part of any stability promise. -`Graph::autotune()` in `EXHAUSTIVE` mode primes every engine advertising the knob: it compiles a *separate* plan carrying `global.benchmarking=1`, executes it once, then discards that plan and compiles and times the plan it keeps from the original knob settings. Discarding is required, because a primed plan re-samples every candidate on each execute and timing it would measure the sampling sweep instead of the kernel. Priming therefore only pays off if its result survives in the provider: MIOpen's search populates a find-db that outlives the throwaway plan. The ingestor keeps its winner only in the plan that measured it, so an `EXHAUSTIVE` run pays `candidates x (warmup + iterations) + 1` extra device executions per spec and then selects by ranked order. `HIPDNN_FORCE_BENCHMARKING=0` suppresses that cost. +`Graph::autotune()` in `EXHAUSTIVE` mode primes every engine advertising the knob: it compiles a *separate* plan carrying `global.benchmarking=1`, executes it once, then discards that plan and compiles and times the plan it keeps from the original knob settings. Discarding is required, because that first execute samples every candidate, and timing it would measure the sampling sweep instead of a single kernel; the ingestor's `BenchmarkPlan` resolves once for the plan's lifetime (see `TheWinnerIsResolvedOnceAcrossRepeatedExecuteCalls`), so this is specifically about the priming execute, not a claim that a benchmarking-enabled plan resamples on every call. Priming therefore only pays off if its result survives in the provider: MIOpen's search populates a find-db that outlives the throwaway plan, and the ingestor's winner cache does the same. The ingestor keeps the measured ranking in its winner cache, keyed by (graph content, device), so the ranking survives the throwaway priming plan. An `EXHAUS… A provider implementing `global.benchmarking` by hand rather than through the ingestor must apply `hipdnn_plugin_sdk::benchmarkingOverrideFromEnv()` as the ingestor and MIOpen do: consulted *outside* any config-validity branch and composed as `override.value_or(knobValue)` -- **never** an OR. An OR cannot express force-off, because a `false` override term never clears a `true` knob term, so `HIPDNN_FORCE_BENCHMARKING=0` could not override a knob-enabled run. diff --git a/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/data_objects/cachekey_generated.h b/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/data_objects/cachekey_generated.h index 79f950d2ed11..2faf6047f8ca 100644 --- a/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/data_objects/cachekey_generated.h +++ b/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/data_objects/cachekey_generated.h @@ -8,6 +8,7 @@ #pragma once #include +#include #include #include @@ -33,7 +34,7 @@ class Hasher { static_assert(std::is_trivially_copyable_v); uint8_t bytes[sizeof(TValue)]; - __builtin_memcpy(bytes, &value, sizeof(TValue)); + std::memcpy(bytes, &value, sizeof(TValue)); for(uint8_t byte : bytes) { tag(byte); @@ -413,9 +414,13 @@ inline bool logicallyEqual(const BFloat16Value* a, const BFloat16Value* b, const { return false; } - if(a->value() != b->value()) { - return false; + const auto aValue = a->value(); + const auto bValue = b->value(); + if(std::memcmp(&aValue, &bValue, sizeof(aValue)) != 0) + { + return false; + } } return true; } @@ -1625,9 +1630,13 @@ inline bool logicallyEqual(const Float16Value* a, const Float16Value* b, const U { return false; } - if(a->value() != b->value()) { - return false; + const auto aValue = a->value(); + const auto bValue = b->value(); + if(std::memcmp(&aValue, &bValue, sizeof(aValue)) != 0) + { + return false; + } } return true; } @@ -1653,9 +1662,13 @@ inline bool logicallyEqual(const Float32Value* a, const Float32Value* b, const U { return false; } - if(a->value() != b->value()) { - return false; + const auto aValue = a->value(); + const auto bValue = b->value(); + if(std::memcmp(&aValue, &bValue, sizeof(aValue)) != 0) + { + return false; + } } return true; } @@ -1681,9 +1694,13 @@ inline bool logicallyEqual(const Float64Value* a, const Float64Value* b, const U { return false; } - if(a->value() != b->value()) { - return false; + const auto aValue = a->value(); + const auto bValue = b->value(); + if(std::memcmp(&aValue, &bValue, sizeof(aValue)) != 0) + { + return false; + } } return true; } @@ -2371,17 +2388,41 @@ inline bool logicallyEqual(const PointwiseAttributes* a, const PointwiseAttribut { return false; } - if(a->relu_lower_clip() != b->relu_lower_clip()) { - return false; + const auto aValue = a->relu_lower_clip(); + const auto bValue = b->relu_lower_clip(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } - if(a->relu_upper_clip() != b->relu_upper_clip()) { - return false; + const auto aValue = a->relu_upper_clip(); + const auto bValue = b->relu_upper_clip(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } - if(a->relu_lower_clip_slope() != b->relu_lower_clip_slope()) { - return false; + const auto aValue = a->relu_lower_clip_slope(); + const auto bValue = b->relu_lower_clip_slope(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } if(a->axis_tensor_uid() != b->axis_tensor_uid()) { @@ -2419,17 +2460,41 @@ inline bool logicallyEqual(const PointwiseAttributes* a, const PointwiseAttribut { return false; } - if(a->swish_beta() != b->swish_beta()) { - return false; + const auto aValue = a->swish_beta(); + const auto bValue = b->swish_beta(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } - if(a->elu_alpha() != b->elu_alpha()) { - return false; + const auto aValue = a->elu_alpha(); + const auto bValue = b->elu_alpha(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } - if(a->softplus_beta() != b->softplus_beta()) { - return false; + const auto aValue = a->softplus_beta(); + const auto bValue = b->softplus_beta(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } return true; } @@ -3587,13 +3652,29 @@ inline bool logicallyEqual(const SdpaAttributes* a, const SdpaAttributes* b, con { return false; } - if(a->dropout_probability() != b->dropout_probability()) { - return false; + const auto aValue = a->dropout_probability(); + const auto bValue = b->dropout_probability(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } - if(a->attn_scale_value() != b->attn_scale_value()) { - return false; + const auto aValue = a->attn_scale_value(); + const auto bValue = b->attn_scale_value(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } if(a->left_bound() != b->left_bound()) { @@ -3940,13 +4021,29 @@ inline bool logicallyEqual(const SdpaBackwardAttributes* a, const SdpaBackwardAt { return false; } - if(a->dropout_probability() != b->dropout_probability()) { - return false; + const auto aValue = a->dropout_probability(); + const auto bValue = b->dropout_probability(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } - if(a->attn_scale_value() != b->attn_scale_value()) { - return false; + const auto aValue = a->attn_scale_value(); + const auto bValue = b->attn_scale_value(); + if(aValue.has_value() != bValue.has_value()) + { + return false; + } + if(aValue.has_value() && std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0) + { + return false; + } } if(a->left_bound() != b->left_bound()) { diff --git a/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/flatbuffer_utilities/GraphContentKey.hpp b/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/flatbuffer_utilities/GraphContentKey.hpp index a72e6b8a9544..aecda178efa6 100644 --- a/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/flatbuffer_utilities/GraphContentKey.hpp +++ b/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/flatbuffer_utilities/GraphContentKey.hpp @@ -39,6 +39,10 @@ namespace hipdnn_flatbuffers_sdk::flatbuffer_utilities /// /// Tensors and nodes compare in vector order, fixed by `IGraph`'s topological-order /// precondition: two construction orders of one logical DAG miss rather than mismatch. +/// +/// A key is usable as an unordered-container key only when `isUsable()` is true. Callers +/// must check that precondition before lookup or insertion: an unusable key is +/// intentionally non-reflexive, so an inserted unusable key can never be found again. class GraphContentKey { public: @@ -291,6 +295,16 @@ class GraphContentKey { return nullptr; } + + // `IGraph::bytes()`'s validity precondition is documented but unenforced, and + // `root()` calls GetRoot on this copy. Verifying here costs one pass over bytes + // this method already copies, and a malformed buffer becomes an unusable key. + ::flatbuffers::Verifier verifier(bytes.data, bytes.size); + if(!hipdnn_flatbuffers_sdk::data_objects::VerifyGraphBuffer(verifier)) + { + return nullptr; + } + return std::make_shared>(bytes.data, bytes.data + bytes.size); } @@ -300,7 +314,7 @@ class GraphContentKey { return nullptr; } - // Verified by GraphWrapper before bytes() would hand them over. + // Verified in retain() before the copy this reads. return ::flatbuffers::GetRoot( _content->data()); } diff --git a/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/flatbuffer_utilities/GraphWrapper.hpp b/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/flatbuffer_utilities/GraphWrapper.hpp index c4972b9cafac..f06fd502f28a 100644 --- a/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/flatbuffer_utilities/GraphWrapper.hpp +++ b/projects/hipdnn/flatbuffers_sdk/include/hipdnn_flatbuffers_sdk/flatbuffer_utilities/GraphWrapper.hpp @@ -38,6 +38,10 @@ class IGraph getTensorMap() const = 0; + // ABI rule: `bytes()` is the last virtual in `IGraph`. Appending after it preserves + // existing caller slots, but separately compiled derived classes are not ABI-safe; + // never insert a virtual before it, or existing vtable slots can change. + /// The verified buffer this graph is a view over; the caller does not own it and /// may copy it to outlive the caller's storage. /// diff --git a/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/ContentCarryingTestGraph.hpp b/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/ContentCarryingTestGraph.hpp index c0605bb2580b..94bd0086a126 100644 --- a/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/ContentCarryingTestGraph.hpp +++ b/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/ContentCarryingTestGraph.hpp @@ -38,6 +38,9 @@ class ContentCarryingTestGraph : public hipdnn_flatbuffers_sdk::flatbuffer_utili std::vector dims{4, 8}; std::vector strides{8, 1}; std::optional raggedOffsetTensorUid = std::nullopt; + /// A required (non-optional) `value: TensorValue` union, so tests can prove the + /// scalar Float memcmp site folds every byte, not just presence/absence. + std::optional value = std::nullopt; }; struct NodeSpec { @@ -49,6 +52,8 @@ class ContentCarryingTestGraph : public hipdnn_flatbuffers_sdk::flatbuffer_utili int64_t in0TensorUid = 1; int64_t out0TensorUid = 2; std::optional in1TensorUid = std::nullopt; + /// A real float knob, so tests can prove float fields key on their bytes. + std::optional reluLowerClip = std::nullopt; }; /// Describes one valid two-tensor, one-node graph by default. @@ -144,13 +149,19 @@ class ContentCarryingTestGraph : public hipdnn_flatbuffers_sdk::flatbuffer_utili void build() { using namespace hipdnn_flatbuffers_sdk::data_objects; - std::vector> tensorOffsets; tensorOffsets.reserve(_spec.tensors.size()); + for(const auto& tensor : _spec.tensors) { auto dims = _builder.CreateVector(tensor.dims); auto strides = _builder.CreateVector(tensor.strides); + flatbuffers::Offset value = 0; + if(tensor.value.has_value()) + { + const Float32Value floatValue(*tensor.value); + value = _builder.CreateStruct(floatValue).Union(); + } TensorAttributesBuilder tensorBuilder(_builder); tensorBuilder.add_uid(tensor.uid); tensorBuilder.add_data_type(tensor.dataType); @@ -160,6 +171,11 @@ class ContentCarryingTestGraph : public hipdnn_flatbuffers_sdk::flatbuffer_utili { tensorBuilder.add_ragged_offset_tensor_uid(*tensor.raggedOffsetTensorUid); } + if(tensor.value.has_value()) + { + tensorBuilder.add_value_type(TensorValue::Float32Value); + tensorBuilder.add_value(value); + } tensorOffsets.push_back(tensorBuilder.Finish()); } @@ -175,6 +191,10 @@ class ContentCarryingTestGraph : public hipdnn_flatbuffers_sdk::flatbuffer_utili { attributesBuilder.add_in_1_tensor_uid(*node.in1TensorUid); } + if(node.reluLowerClip.has_value()) + { + attributesBuilder.add_relu_lower_clip(*node.reluLowerClip); + } const auto attributes = attributesBuilder.Finish(); auto nodeName = _builder.CreateString(node.name); diff --git a/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/TestGraphContentKey.cpp b/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/TestGraphContentKey.cpp index 81102d5d8b19..404353fb74d6 100644 --- a/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/TestGraphContentKey.cpp +++ b/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/TestGraphContentKey.cpp @@ -5,7 +5,9 @@ #include +#include #include +#include #include "ContentCarryingTestGraph.hpp" @@ -356,6 +358,38 @@ TEST(TestGraphContentKey, ADifferentPointwiseOperationComparesUnequal) keyFor(ContentCarryingTestGraph{multiplication})); } +/// A float field hashes its raw bytes, so it must compare its raw bytes too. Under IEEE +/// `!=` a NaN payload is unequal to itself: the two keys below would hash into the same +/// bucket and then refuse to match, making that graph a permanent cache miss. +TEST(TestGraphContentKey, TwoGraphsCarryingTheSameNanFloatComparePairwiseEqual) +{ + Spec first; + first.nodes[0].reluLowerClip = std::numeric_limits::quiet_NaN(); + const Spec second = first; + + // Distinct keys over separately built buffers: `operator==`'s pointer fast path + // would answer a self-comparison without reaching the generated compare. + const auto left = keyFor(ContentCarryingTestGraph{first}); + const auto right = keyFor(ContentCarryingTestGraph{second}); + + ASSERT_TRUE(left.isUsable()); + EXPECT_EQ(left.hash(), right.hash()); + EXPECT_EQ(left, right) << "identical NaN bits must key the same, or the graph can " + "never be found in the cache it was written to"; +} + +/// A different NaN payload is different content, matching the raw-byte hash that +/// already splits it. +TEST(TestGraphContentKey, TwoGraphsCarryingDifferentNanPayloadsCompareUnequal) +{ + Spec quiet; + quiet.nodes[0].reluLowerClip = std::numeric_limits::quiet_NaN(); + Spec signaling; + signaling.nodes[0].reluLowerClip = std::numeric_limits::signaling_NaN(); + + EXPECT_NE(keyFor(ContentCarryingTestGraph{quiet}), keyFor(ContentCarryingTestGraph{signaling})); +} + /// The graph-level data types are defaults: the frontend stamps them onto each node and /// tensor left unset (Attributes::fill_from_context), and those are compared. Folding /// them would split the key between an explicit type and the same type defaulted. @@ -520,6 +554,36 @@ TEST(TestGraphContentKey, AGraphWithNoBytesYieldsAnUnusableKey) EXPECT_FALSE(GraphContentKey{graph}.isUsable()); } +/// `bytes()` promises a verified buffer but nothing enforces it, and `root()` calls +/// `GetRoot` on the retained copy. A malformed buffer must become an unusable key, not +/// an out-of-bounds read through a fabricated vtable of offsets. +class MalformedBytesGraph : public UnkeyableGraph +{ +public: + bool isValid() const override + { + return true; + } + + hipdnn_flatbuffers_sdk::flatbuffer_utilities::SerializedBlobView bytes() const override + { + return {_garbage.data(), _garbage.size()}; + } + +private: + // Non-empty and wrong: the leading word reads as a root offset far past the end. + std::vector _garbage{0xFFU, 0xFFU, 0xFFU, 0x7FU, 0x01U, 0x02U, 0x03U, 0x04U}; +}; + +TEST(TestGraphContentKey, AGraphWithMalformedBytesYieldsAnUnusableKey) +{ + const MalformedBytesGraph graph; + const GraphContentKey key{graph}; + + EXPECT_FALSE(key.isUsable()); + EXPECT_EQ(key.hash(), 0U) << "an unverifiable buffer must not be filed under a live hash"; +} + /// Hash 0, unusable, and matching nothing are one fact: a key with a live hash but no /// content would be bucketable yet not equal to itself. TEST(TestGraphContentKey, AnUnusableKeyHashesToZero) diff --git a/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/TestGraphWrapper.cpp b/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/TestGraphWrapper.cpp index 3f2d2e7fa371..c56c578deb55 100644 --- a/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/TestGraphWrapper.cpp +++ b/projects/hipdnn/flatbuffers_sdk/tests/flatbuffer_utilities/TestGraphWrapper.cpp @@ -148,9 +148,8 @@ TEST(TestGraphWrapper, GetNodeWrapper) EXPECT_THROW(wrapper.getNodeWrapper(1), std::out_of_range); } -/// `bytes()` is the cache-key seam: `GraphContentKey` reads the buffer back through it -/// and calls `GetRoot` without re-verifying, so a buffer is retained only once the -/// verifier has accepted it. +/// `bytes()` is the cache-key seam: `GraphContentKey` verifies the retained buffer before +/// calling `GetRoot`, so a buffer is retained only once the verifier has accepted it. TEST(TestGraphWrapper, BytesReturnsTheVerifiedBuffer) { flatbuffers::FlatBufferBuilder builder diff --git a/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/BenchmarkPlan.hpp b/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/BenchmarkPlan.hpp index 5098a6a1b2ff..804b9052f085 100644 --- a/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/BenchmarkPlan.hpp +++ b/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/BenchmarkPlan.hpp @@ -293,7 +293,6 @@ class BenchmarkPlan : public IPlan return best; } -protected: /// The representative time of BENCHMARK_ITERATIONS timed executes, after /// BENCHMARK_WARMUP_RUNS untimed ones. Returns nullopt if the candidate threw or could /// not be timed; both score the candidate unusable rather than throwing out of diff --git a/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/GenericPlanBuilder.hpp b/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/GenericPlanBuilder.hpp index e7d1e0ac9100..a639d5f884df 100644 --- a/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/GenericPlanBuilder.hpp +++ b/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/GenericPlanBuilder.hpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -183,10 +184,17 @@ class GenericPlanBuilder : public IPlanBuilder // Coverage and orderability are checked against the knob-filtered candidates // here, independent of the same check against the full catalog in // sortedCatalog(): one can fail while the other passes. - const WinnerKey winnerKey{ - hipdnn_flatbuffers_sdk::flatbuffer_utilities::GraphContentKey{opGraph}, - DeviceKey{context.deviceProperties}}; - const auto record = _stateManager.winnerFor(winnerKey); + std::optional winnerKey; + std::optional record; + if(settings.benchmarkingEnabled + || _stateManager.mightHaveWinnerFor(context.deviceProperties.gcnArchName)) + { + winnerKey + = WinnerKey{hipdnn_flatbuffers_sdk::flatbuffer_utilities::GraphContentKey{opGraph}, + DeviceKey{context.deviceProperties}}; + record = _stateManager.winnerFor(*winnerKey); + } + if(record.has_value()) { if(const auto ranked = orderIfFullyCovered(*record, filtered); ranked.has_value()) @@ -325,13 +333,13 @@ class GenericPlanBuilder : public IPlanBuilder // append rather than adopt. const auto cause = record.has_value() ? WinnerWriteCause::COVERAGE_REBENCHMARK : WinnerWriteCause::FRESH_MISS; - executionContext.setPlan( - makeBenchmarkPlan(std::move(candidates), - handle, - [&stateManager = _stateManager, winnerKey, cause]( - const std::vector& ranking) { - stateManager.recordWinner(winnerKey, ranking, cause); - })); + executionContext.setPlan(makeBenchmarkPlan( + std::move(candidates), + handle, + [&stateManager = _stateManager, winnerKey = std::move(*winnerKey), cause]( + const std::vector& ranking) { + stateManager.recordWinner(winnerKey, ranking, cause); + })); } /// One knob per KMD field the engine exposes; default is the top-ranked value. std::vector diff --git a/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/KernelIngestorStateManager.hpp b/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/KernelIngestorStateManager.hpp index b3dfcdf9f905..47a2951650c7 100644 --- a/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/KernelIngestorStateManager.hpp +++ b/projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/KernelIngestorStateManager.hpp @@ -207,6 +207,15 @@ class KernelIngestorStateManager /// `rank()` is never called; otherwise the heuristic orders it. Catalog sortedCatalog(const MatchContext& context) const { + // Mirrors catalogFor's own reject guard: cacheKey() below only reads graph and + // device ordinal, not arch, so without this an unresolved-arch context would + // cache an empty catalog under the SAME key a later, resolved call for this + // device reuses -- permanently hiding that device's real catalog. + if(context.deviceId == NO_DEVICE || context.deviceProperties.gcnArchName.empty()) + { + return catalogFor(context); + } + Catalog catalog = catalogFor(context); // A measured order is final; a heuristic one is provisional, so this lookup runs @@ -326,6 +335,17 @@ class KernelIngestorStateManager return _winnerCache.size(); } + /// Is it worth building a WinnerKey for @p gcnArchName? True if the in-memory cache + /// holds anything, or this arch's shard has not been attempted yet. Probes the + /// stripped arch, matching how `loadShardIfAbsent()` latches. + bool mightHaveWinnerFor(const std::string& gcnArchName) const + { + const std::lock_guard guard(_winnerCacheMutex); + return !_winnerCache.empty() + || _loadedWinnerShards.find(std::string(stripArchFeatures(gcnArchName))) + == _loadedWinnerShards.end(); + } + /// Resolves how to size and launch @p kernel. /// @throws std::runtime_error if the kernel's dispatch descriptor is unknown. KernelDispatcher getDispatchDetails(const KernelDefinition& kernel) const @@ -515,9 +535,12 @@ class KernelIngestorStateManager // Pack pruning and matchers both read the device, so nothing below can be // answered without one. Checked here rather than in every provider's matchers, // where an omission is invisible. - if(context.deviceId == NO_DEVICE) + if(context.deviceId == NO_DEVICE || context.deviceProperties.gcnArchName.empty()) { - HIPDNN_PLUGIN_LOG_INFO("ingestor: no device resolved; no kernel applies"); + const auto* reason = context.deviceId == NO_DEVICE + ? "no device resolved" + : "resolved device reports no gcnArchName"; + HIPDNN_PLUGIN_LOG_INFO("ingestor: " << reason << "; no kernel applies"); return Catalog{}; } @@ -745,17 +768,6 @@ class KernelIngestorStateManager return ordered; } - /// Is it worth building a WinnerKey for @p gcnArchName? True if the in-memory cache - /// holds anything, or this arch's shard has not been attempted yet. Probes the - /// stripped arch, matching how `loadShardIfAbsent()` latches. - bool mightHaveWinnerFor(const std::string& gcnArchName) const - { - const std::lock_guard guard(_winnerCacheMutex); - return !_winnerCache.empty() - || _loadedWinnerShards.find(std::string(stripArchFeatures(gcnArchName))) - == _loadedWinnerShards.end(); - } - /// Loads the on-disk shard covering @p gcnArchName into `_winnerCache` once, tracked /// by `_loadedWinnerShards`. File I/O runs with `_winnerCacheMutex` UNHELD, so a slow /// disk read never blocks an unrelated call. diff --git a/projects/hipdnn/plugin_sdk/tests/ingestor/TestGenericPlanBuilder.cpp b/projects/hipdnn/plugin_sdk/tests/ingestor/TestGenericPlanBuilder.cpp index 1066fa461c3f..5d0af354af23 100644 --- a/projects/hipdnn/plugin_sdk/tests/ingestor/TestGenericPlanBuilder.cpp +++ b/projects/hipdnn/plugin_sdk/tests/ingestor/TestGenericPlanBuilder.cpp @@ -4,6 +4,7 @@ #ifdef HIPDNN_ENABLE_KERNEL_INGESTOR #include +#include #include #include #include @@ -52,6 +53,26 @@ using namespace hipdnn_plugin_sdk::ingestor; using namespace hipdnn_plugin_sdk::ingestor::testing; using hipdnn_flatbuffers_sdk::flatbuffer_utilities::GraphContentKey; using hipdnn_flatbuffers_sdk::flatbuffer_utilities::testing::ContentCarryingTestGraph; + +class CountingBytesGraph final + : public hipdnn_flatbuffers_sdk::flatbuffer_utilities::testing::ContentCarryingTestGraph +{ +public: + explicit CountingBytesGraph(std::shared_ptr bytesCalls) + : _bytesCalls(std::move(bytesCalls)) + { + } + + hipdnn_flatbuffers_sdk::flatbuffer_utilities::SerializedBlobView bytes() const override + { + ++*_bytesCalls; + return ContentCarryingTestGraph::bytes(); + } + +private: + std::shared_ptr _bytesCalls; +}; + using ::testing::_; using ::testing::Ref; using ::testing::Return; @@ -1040,6 +1061,38 @@ TEST_F(TestIngestorGenericPlanBuilderBenchmarking, EXPECT_EQ(context.plan().kernel().getIntMetadata(BLOCK_SIZE), 64); } +TEST(TestIngestorGenericPlanBuilder, ASecondColdMissWithBenchmarkingOffDoesNotReadGraphBytes) +{ + const ScopedSymbols symbols("test.graph", acceptGraph, "test.kernel", countingFloatKernels); + const ScopedConstantScore constantScore; + const WorkspaceEqualsBlockSizeHandler handler; + const ScopedDispatchRegistration dispatch("test.dispatch", handler); + const auto manager = makeThreeKernelWorkspaceStateManager(); + const auto engine = makeEngineWithKnobs({BLOCK_SIZE}); + const TestDeviceResolver resolver; + const TestPlanBuilder builder(engine, *manager, resolver); + + flatbuffers::FlatBufferBuilder fbb; + const auto engineConfig = makeEmptyEngineConfig(fbb); + const auto bytesCalls = std::make_shared(0); + const CountingBytesGraph graph(bytesCalls); + + KnobFilterSettings settings; + builder.initializeExecutionSettings(0, graph, engineConfig, settings); + ASSERT_FALSE(settings.ingestorSettings.benchmarkingEnabled); + + KnobFilterContext context; + context.setExecutionSettings(settings); + builder.buildPlan(0, graph, engineConfig, context); + + bytesCalls->store(0); + builder.buildPlan(0, graph, engineConfig, context); + + EXPECT_EQ(context.plan().kernel().getIntMetadata(BLOCK_SIZE), 64); + EXPECT_EQ(bytesCalls->load(), 0U) + << "a second cold miss with benchmarking off must not read graph bytes"; +} + /// prepare() fails for one kernel and succeeds for the rest: the shape of a code object /// that cannot be loaded -- a kpack archive missing from the install, a symbol the module /// does not export -- which is a property of that one kernel, not of its pack. diff --git a/projects/hipdnn/plugin_sdk/tests/ingestor/TestWinnerCache.cpp b/projects/hipdnn/plugin_sdk/tests/ingestor/TestWinnerCache.cpp index 971001dadc03..547489d7a3c9 100644 --- a/projects/hipdnn/plugin_sdk/tests/ingestor/TestWinnerCache.cpp +++ b/projects/hipdnn/plugin_sdk/tests/ingestor/TestWinnerCache.cpp @@ -433,6 +433,33 @@ TEST(TestIngestorWinnerCacheStateManager, ACoveringRecordOrdersTheCatalogWithout << "a covering record must decide the order, not the heuristic"; } +/// A resolved device that names no arch cannot be told apart from another one: folding +/// two of them into one winner-cache key would serve a measurement taken elsewhere. +/// `GenericPlanBuilder::contextFor` throws on this, so the guard here defends callers +/// that build a MatchContext directly. +TEST(TestIngestorWinnerCacheStateManager, AnEmptyArchNameYieldsAnEmptyCatalog) +{ + const ScopedSymbols symbols("test.graph", acceptGraph, "test.kernel", countingFloatKernels); + const auto manager = makeStateManager(); + const TestGraph graph(makeGraphId(0xE9)); + + auto properties = testDeviceProperties(); + ASSERT_FALSE(manager->sortedDefinitions(MatchContext{graph, 0, properties}).empty()) + << "this test needs a catalog that is non-empty when the arch is named"; + + properties.gcnArchName.clear(); + + EXPECT_TRUE(manager->sortedDefinitions(MatchContext{graph, 0, properties}).empty()) + << "an unidentified device must match no kernel"; + + // The catalog cache is keyed on (graph, device ordinal), not arch: an empty-arch + // lookup that wrote its rejected, empty catalog under that key would permanently + // hide this device's real catalog once the arch is known again. + properties.gcnArchName = testDeviceProperties().gcnArchName; + EXPECT_FALSE(manager->sortedDefinitions(MatchContext{graph, 0, properties}).empty()) + << "an unresolved-arch lookup must not poison this device's cached catalog"; +} + /// The production sequence, on ONE manager: sort (heuristic, memoized), then record, /// then sort again. A benchmark sweep always writes after buildPlan already sorted and /// cached the catalog, so a memoized heuristic order must yield to a later measurement. @@ -498,8 +525,8 @@ TEST(TestIngestorWinnerCacheStateManager, APartialRecordLeavesTheHeuristicOrderI << "an uncovering record must not reorder anything"; } -/// The cache sits under its own mutex. Concurrent readers and writers must neither race -/// nor lose entries; this is the guard for someone removing the lock as "unneeded". +/// Stress concurrent readers and writers. These assertions check entry survival and +/// whole-record reads; they do not prove race freedom without ThreadSanitizer. TEST(TestIngestorWinnerCacheStateManager, ConcurrentWritersAndReadersKeepEveryEntry) { const ScopedSymbols symbols("test.graph", acceptGraph, "test.kernel", countingFloatKernels); diff --git a/projects/hipdnn/scripts/gen_cache_key.py b/projects/hipdnn/scripts/gen_cache_key.py index 9c89d923f8f2..15526ebbd19d 100644 --- a/projects/hipdnn/scripts/gen_cache_key.py +++ b/projects/hipdnn/scripts/gen_cache_key.py @@ -286,6 +286,31 @@ def _resolve_domain(self): f"ERROR: {UID_DOMAIN_ATTRIBUTE} is declared on {len(domains)} fields " f"({names}); ordinals need exactly one" ) + candidates = [] + for candidate in self.fields_of(root): + candidate_type = candidate["type"] + if ( + candidate_type["base_type"] != "Vector" + or candidate_type.get("element") != "Obj" + ): + continue + candidate_index = candidate_type.get("index", -1) + if candidate_index < 0: + continue + candidate_element = self.schema["objects"][candidate_index] + candidate_keys = [ + f for f in candidate_element["fields"] if self.is_uid_key(f) + ] + if len(candidate_keys) == 1 and self._is_integer_scalar( + candidate_keys[0]["type"] + ): + candidates.append(candidate) + if len(candidates) > 1: + names = ", ".join(f"'{f['name']}'" for f in candidates) + raise SystemExit( + f"ERROR: {UID_DOMAIN_ATTRIBUTE} has {len(candidates)} domain-shaped " + f"candidates ({names}); ordinals need exactly one" + ) field = domains[0] ftype = field["type"] if ftype["base_type"] != "Vector" or ftype.get("element") != "Obj": @@ -293,7 +318,13 @@ def _resolve_domain(self): f"ERROR: {UID_DOMAIN_ATTRIBUTE} on '{field['name']}' requires a vector " "of tables" ) - element = self.schema["objects"][ftype.get("index", -1)] + element_index = ftype.get("index", -1) + if element_index < 0: + raise SystemExit( + f"ERROR: {UID_DOMAIN_ATTRIBUTE} on '{field['name']}' has no element " + "index; the schema does not name its table" + ) + element = self.schema["objects"][element_index] keys = [f for f in element["fields"] if self.is_uid_key(f)] if len(keys) != 1: raise SystemExit( @@ -470,6 +501,7 @@ def emit_prologue(self, namespace): self.w("#pragma once") self.w("") self.w("#include ") + self.w("#include ") self.w("#include ") self.w("#include ") self.w("") @@ -497,7 +529,7 @@ def emit_prologue(self, namespace): self.w(" {") self.w(" static_assert(std::is_trivially_copyable_v);") self.w(" uint8_t bytes[sizeof(TValue)];") - self.w(" __builtin_memcpy(bytes, &value, sizeof(TValue));") + self.w(" std::memcpy(bytes, &value, sizeof(TValue));") self.w(" for(uint8_t byte : bytes)") self.w(" {") self.w(" tag(byte);") @@ -847,6 +879,15 @@ def emit_equal_field(self, field, owner): self.w(" {") self.w(" return false;") self.w(" }") + elif element in ("Float", "Double"): + self.w(" const auto aValue = aItems->Get(index);") + self.w(" const auto bValue = bItems->Get(index);") + self.w( + " if(std::memcmp(&aValue, &bValue, sizeof(aValue)) != 0)" + ) + self.w(" {") + self.w(" return false;") + self.w(" }") else: if uid: self.w( @@ -907,6 +948,27 @@ def emit_equal_field(self, field, owner): self.w(" {") self.w(" return false;") self.w(" }") + elif base in ("Float", "Double"): + # Floats hash bitwise, so they must compare bitwise: IEEE `!=` makes a NaN + # payload unequal to itself and turns its cache entry into a permanent miss. + self.w(" {") + self.w(f" const auto aValue = {left};") + self.w(f" const auto bValue = {right};") + if field.get("optional"): + self.w(" if(aValue.has_value() != bValue.has_value())") + self.w(" {") + self.w(" return false;") + self.w(" }") + self.w( + " if(aValue.has_value() && " + "std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0)" + ) + else: + self.w(" if(std::memcmp(&aValue, &bValue, sizeof(aValue)) != 0)") + self.w(" {") + self.w(" return false;") + self.w(" }") + self.w(" }") elif base in SCALAR_BASE_TYPES: self.w(f" if({left} != {right})") self.w(" {") diff --git a/projects/hipdnn/scripts/test_gen_cache_key.py b/projects/hipdnn/scripts/test_gen_cache_key.py index 73d154e1a453..21e973b4e7b6 100755 --- a/projects/hipdnn/scripts/test_gen_cache_key.py +++ b/projects/hipdnn/scripts/test_gen_cache_key.py @@ -196,11 +196,29 @@ def test_a_domain_without_exactly_one_key_is_rejected(self): with self.assertRaises(SystemExit): Emitter(broken, f"{NS}.Root") - def test_more_than_one_domain_is_rejected(self): - # Ordinals are positions in one vector; two candidates have no defined answer. + def test_more_than_one_domain_annotation_is_rejected(self): + # The second annotation is on a non-domain-shaped vector, so this reaches + # the duplicate-annotation check rather than the candidate ambiguity check. broken = uid_schema([field("x_tensor_uid", "Long", 0, uid=True)]) + broken["objects"].append(table(f"{NS}.Decoy", [field("value", "Int", 0)])) broken["objects"][2]["fields"].append( - field("more_tensors", "Vector", 2, index=0, element="Obj", uid_domain=True) + field("more_domains", "Vector", 2, index=3, element="Obj", uid_domain=True) + ) + with self.assertRaisesRegex(SystemExit, "declared on 2 fields"): + Emitter(broken, f"{NS}.Root") + + def test_a_second_domain_shaped_candidate_is_rejected(self): + # The annotation alone cannot disambiguate: a second root vector of tables with + # one integer key is equally domain-shaped, so the annotated one may be the + # wrong one and every ordinal would fold against it silently. + broken = uid_schema( + [field("x_tensor_uid", "Long", 0, uid=True)], domain_ignored=True + ) + broken["objects"].append( + table(f"{NS}.Decoy", [field("uid", "Long", 0, uid_key=True)]) + ) + broken["objects"][2]["fields"].append( + field("decoys", "Vector", 2, index=3, element="Obj", uid_domain=True) ) with self.assertRaises(SystemExit): Emitter(broken, f"{NS}.Root") @@ -271,6 +289,75 @@ def test_the_prime_matches_the_canonical_fnv_1a_64_constant(self): ) +class TestPortableByteOperations(unittest.TestCase): + """The emitted header compiles under MSVC and compares floats by their bytes. + + `__builtin_memcpy` is a GCC/Clang extension, and IEEE `!=` makes a NaN payload + unequal to itself, which would turn its cache entry into a permanent miss. + """ + + def test_the_hasher_uses_standard_memcpy(self): + root = table(f"{NS}.Root", [field("alpha", "Int", 0)]) + header = Emitter(schema([root]), f"{NS}.Root").emit() + self.assertIn("#include ", header) + self.assertIn("std::memcpy(", header) + self.assertNotIn("__builtin_memcpy", header) + + def test_a_float_field_compares_its_object_representation(self): + root = table(f"{NS}.Root", [field("alpha", "Float", 0)]) + bodies = function_bodies(Emitter(schema([root]), f"{NS}.Root").emit()) + self.assertIn( + "std::memcmp(&aValue, &bValue, sizeof(aValue)) != 0", + bodies[("equal", "Root")], + ) + + def test_an_optional_double_keeps_its_presence_tag(self): + root = table(f"{NS}.Root", [field("beta", "Double", 0, optional=True)]) + body = function_bodies(Emitter(schema([root]), f"{NS}.Root").emit())[ + ("equal", "Root") + ] + self.assertIn("aValue.has_value() != bValue.has_value()", body) + self.assertIn("std::memcmp(&*aValue, &*bValue, sizeof(*aValue)) != 0", body) + + def test_a_float_vector_compares_elements_by_object_representation(self): + root = table( + f"{NS}.Root", + [field("values", "Vector", 0, element="Float")], + ) + body = function_bodies(Emitter(schema([root]), f"{NS}.Root").emit())[ + ("equal", "Root") + ] + self.assertIn("std::memcmp(&aValue, &bValue, sizeof(aValue)) != 0", body) + self.assertNotIn( + "aItems->Get(index) != bItems->Get(index)", + body, + ) + + +class TestEmittedBodiesAreWellFormed(unittest.TestCase): + """Every vector element kind must emit balanced braces. + + A branch that drops its closing brace still passes an `assertIn` on the body + text, but the header it emits does not compile. Brace balance is the property + worth asserting, over every element kind the emitter has a branch for. + """ + + ELEMENT_KINDS = ["String", "Float", "Double", "Int", "Long", "UByte", "Bool"] + + def test_a_vector_of_each_element_kind_emits_balanced_braces(self): + for element in self.ELEMENT_KINDS: + with self.subTest(element=element): + root = table( + f"{NS}.Root", [field("values", "Vector", 0, element=element)] + ) + header = Emitter(schema([root]), f"{NS}.Root").emit() + self.assertEqual( + header.count("{"), + header.count("}"), + f"a [{element}] vector emits unbalanced braces; the header will not compile", + ) + + class TestUnhandledBaseType(unittest.TestCase): """A base type the generator has no fold rule for must fail loudly.