diff --git a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp index 354369ba2bd6..af4c17a959a2 100644 --- a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp +++ b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -107,8 +108,25 @@ static std::string unsupported_node_message(const Graph & graph, size_t index, c // is_layout_alias_node() is this predicate exactly (graph/graph.cpp): a message that restates the scheduler's rule // is a message that will describe a scheduler that no longer exists. Same reasoning as rope-claim-params.h sharing // its predicates with the matchers in #257. - stream << " alias=" << (is_layout_alias_node(graph, node) ? "same_storage" : "not_aliasing_input") - << " inputs=" << node.inputs.size(); + // `aliasing` is the decision; the string is derived from it, never the other way round (review of #262: the + // first version compared the string it had just built to decide whether to look further). + const bool aliasing = is_layout_alias_node(graph, node); + const char * relation = aliasing ? "same_storage" : "not_aliasing_input"; + // WHICH lookup missed, read from the VALUE, not re-asked of the map. The map has grown since the storage was + // assigned, so `find_tensor(view_src)` here can succeed where it failed then and this message would name the + // wrong repair. AliasMiss is recorded at assignment time for exactly this (value-map.h), and it is the half of + // this PR that the review called blocking. + const char * why = ""; + const Value * out = graph.values().find(node.output); + if (!aliasing && out != nullptr) { + switch (out->alias_miss) { + case AliasMiss::NoAliasSource: why = "(no_alias_source)"; break; + case AliasMiss::ExactViewSrc: why = "(aliased_view_src_not_the_input)"; break; + case AliasMiss::RangeFallback: why = "(aliased_storage_root_fallback_not_the_input)"; break; + case AliasMiss::None: break; + } + } + stream << " alias=" << relation << why << " inputs=" << node.inputs.size(); } return stream.str(); } diff --git a/ggml/src/ggml-hrx/graph/graph-diagnostics.cpp b/ggml/src/ggml-hrx/graph/graph-diagnostics.cpp index 375852800f0d..bf3b946d40db 100644 --- a/ggml/src/ggml-hrx/graph/graph-diagnostics.cpp +++ b/ggml/src/ggml-hrx/graph/graph-diagnostics.cpp @@ -455,6 +455,7 @@ GraphSnapshotLoadResult load_graph_snapshot_json(const std::string & contents) { item.at("byte_count").get(), item.at("contiguous").get(), nullptr, + AliasMiss::None, std::nullopt, }; if (!parse_value_kind(item.at("kind").get(), value.kind)) { diff --git a/ggml/src/ggml-hrx/graph/value-map.cpp b/ggml/src/ggml-hrx/graph/value-map.cpp index 8f37c6d3abc9..ba4f317bca3c 100644 --- a/ggml/src/ggml-hrx/graph/value-map.cpp +++ b/ggml/src/ggml-hrx/graph/value-map.cpp @@ -90,12 +90,15 @@ ValueId ValueMap::get_or_add_tensor_value(const ggml_tensor * tensor, ValueKind ValueId storage_root; ValueId alias_source_id; size_t storage_offset = 0; + AliasMiss alias_miss = AliasMiss::None; size_t storage_byte_count = ggml_nbytes(tensor); if (alias_source != nullptr) { size_t relative_offset = 0; if (!tensor_storage_relative_offset(alias_source->tensor, tensor, relative_offset)) { relative_offset = tensor->view_offs; } + // which way find_alias_source() found it: exact view_src identity, or the storage-root fallback + alias_miss = find_tensor(tensor->view_src) != nullptr ? AliasMiss::ExactViewSrc : AliasMiss::RangeFallback; storage = alias_source->storage; storage_root = alias_source->storage_root; alias_source_id = alias_source->id; @@ -112,6 +115,11 @@ ValueId ValueMap::get_or_add_tensor_value(const ggml_tensor * tensor, ValueKind storage = ValueStorageId(static_cast(storages_.size())); storage_root = id; storages_.push_back({ storage, storage_root, storage_byte_count }); + // Record the reason HERE, while it is still true: see AliasMiss in value-map.h. No alias source was found, + // so this is the only case the branch can produce - a registered view_src is an early return above. + if (tensor->view_src != nullptr) { + alias_miss = AliasMiss::NoAliasSource; + } } Value value = { @@ -129,6 +137,7 @@ ValueId ValueMap::get_or_add_tensor_value(const ggml_tensor * tensor, ValueKind ggml_nbytes(tensor), ggml_is_contiguous(tensor), tensor, + alias_miss, std::nullopt, }; for (int i = 0; i < GGML_MAX_DIMS; ++i) { diff --git a/ggml/src/ggml-hrx/graph/value-map.h b/ggml/src/ggml-hrx/graph/value-map.h index b026de65fde6..380ddd895ede 100644 --- a/ggml/src/ggml-hrx/graph/value-map.h +++ b/ggml/src/ggml-hrx/graph/value-map.h @@ -47,6 +47,19 @@ inline bool operator!=(ValueStorageId lhs, ValueStorageId rhs) { return !(lhs == rhs); } +// Recorded when get_or_add_tensor_value() creates a storage rather than aliasing (review of #262). +// How the alias source was (or was not) found, recorded when the storage is chosen. The two "found" cases are the +// interesting ones: with an alias source the view joins *that* value's storage, which need not be the storage of the +// node's own input - and that is what makes is_layout_alias_node() false while an alias source exists at all +// (review of #262: the earlier ViewSrcRegisteredStorageDiffers label was unreachable, because a registered view_src is +// an early return in find_alias_source()). +enum class AliasMiss : uint8_t { + None, // not a view, so there is nothing to alias + NoAliasSource, // view_src unregistered and no value shares its storage root: a storage of its own + ExactViewSrc, // the alias source was the view_src value itself + RangeFallback, // the alias source was found by the storage-root fallback, not by identity +}; + enum class ValueKind : uint8_t { External, Transient, @@ -82,6 +95,11 @@ struct Value { size_t byte_count = 0; bool contiguous = false; const ggml_tensor * tensor = nullptr; + // Why this value was given a storage of its own instead of aliasing its input, recorded AT ASSIGNMENT TIME. + // A diagnostic that asks the map later gets a different answer: values added between the assignment and the + // message make `find_tensor(view_src)` succeed where it failed then, and the message would then name the wrong + // repair (review of #262). + AliasMiss alias_miss = AliasMiss::None; std::optional buffer; };