[Mlir-commits] [mlir] [mlir] Add searchable remark history and query-based links (PR #237762)
Guray Ozen
llvmlistbot at llvm.org
Tue Nov 3 05:12:38 PST 2026
https://github.com/grypp updated https://github.com/llvm/llvm-project/pull/237762
>From 0f00bce6a7390bacf23585a31b7b288524eb4d7e Mon Sep 17 00:00:00 2001
From: Guray Ozen <gozen at nvidia.com>
Date: Sun, 1 Nov 2026 19:43:51 +0100
Subject: [PATCH 1/4] [mlir] Add searchable remark history and query-based
links
---
mlir/docs/Remarks.md | 85 +++++++++
mlir/include/mlir/IR/Remarks.h | 113 ++++++++++--
mlir/lib/IR/Remarks.cpp | 83 +++++++++
mlir/unittests/IR/RemarkTest.cpp | 298 +++++++++++++++++++++++++++++++
4 files changed, 562 insertions(+), 17 deletions(-)
diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md
index e747f7bc8f5958..663eb1161e9ad8 100644
--- a/mlir/docs/Remarks.md
+++ b/mlir/docs/Remarks.md
@@ -251,6 +251,91 @@ if (engine->isRemarkEnabled(remark::RemarkKind::RemarkPassed, "Vectorizer"))
***
+## Searching and Linking Remarks
+
+Remarks can refer to other remarks by ID using `RemarkOpts::relatedTo(id)`.
+When the producer does not have the ID, it can search previously reported
+remarks and link to the matches.
+
+First, enable history on an already configured engine:
+
+```c++
+auto *engine = context.getRemarkEngine();
+engine->enableHistory();
+```
+
+History is disabled by default. Enabling it retains the IDs and identifying
+metadata of subsequent reports, without retaining their messages or arguments.
+It does not recover earlier reports or remarks excluded by the engine's
+category and kind filters. An in-flight remark becomes searchable when it is
+reported at the end of its lifetime.
+
+Use `RemarkQuery` to match a name, base category, subcategory, function, kind,
+or location. Specified fields are combined with AND and match exactly, without
+regular expressions. Unspecified fields match anything; an explicitly empty
+string matches empty metadata. Location matching compares the complete MLIR
+location, including wrappers, and does not identify a unique operation.
+
+```c++
+auto analysisOpts = remark::RemarkOpts::name("VectorSizeInference")
+ .category("Vectorizer")
+ .function("kernel");
+remark::analysis(loc, analysisOpts)
+ << remark::reason("Unable to infer vector sizes");
+
+auto query = remark::RemarkQuery()
+ .name("VectorSizeInference")
+ .category("Vectorizer")
+ .function("kernel")
+ .kind(remark::RemarkKind::RemarkAnalysis)
+ .at(loc);
+
+// Obtain the IDs explicitly, in report order.
+SmallVector<remark::RemarkId> matches = engine->findRemarks(query);
+
+// Or link all matching reports without handling their IDs.
+remark::missed(loc, remark::RemarkOpts::name("Vectorization")
+ .category("Vectorizer")
+ .function("kernel"))
+ << remark::reason("Cannot vectorize without vector sizes")
+ << remark::relatedTo(query);
+```
+
+The helper resolves the query when inserted with `<<`. Later reports do not
+change the links. No matches, including when history is disabled, adds no
+links. Links use the existing repeated `RelatedTo` arguments in serialized
+output. Consumers must preserve repeated keys. A query match indicates a
+relationship, not causality; the producer is responsible for choosing relevant
+criteria. There is no automatic selection of the latest match or inference of
+whether earlier evidence is still valid for changed IR.
+
+Queries own their strings, so they can be retained and reused. A query with a
+location must not outlive that location's `MLIRContext`. History access is
+synchronized, and streamer callbacks may query the history, including the
+report being delivered. This does not synchronize the emitting policy or
+streamer. Report order follows insertion into history and can differ from ID
+creation order, including when remarks are emitted concurrently.
+
+History is independent of the output policy: both `All` and `Final` record
+every report, and finalizing a policy does not clear history. Use `All` when
+the output must contain every referenced event. `Final` may deduplicate a
+referenced report or emit it in an earlier batch, so finding its ID in history
+does not guarantee that the current output batch contains it.
+
+History grows with the number of reports until explicitly cleared, disabled,
+or the engine is destroyed. Clear it between compilations when reusing an
+engine:
+
+```c++
+engine->clearHistory(); // Keep recording subsequent reports.
+engine->enableHistory(false); // Discard history and stop recording.
+```
+
+Neither operation resets the ID counter or changes existing links. Repeated
+calls to `enableHistory()` preserve already recorded history.
+
+***
+
## Enabling Remarks
### Option 1: LLVM Remark Streamer (YAML or Bitstream)
diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 0ecc63717b5aee..6acbc722944ff4 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -24,6 +24,7 @@
#include "mlir/IR/Value.h"
#include <atomic>
+#include <mutex>
namespace mlir::remark {
@@ -89,6 +90,7 @@ using namespace llvm;
namespace detail {
class InFlightRemark; // forward declaration
+class RemarkEngine;
} // namespace detail
/// Options to create a Remark
@@ -139,6 +141,52 @@ struct RemarkOpts {
inline RemarkOpts relatedTo(const detail::InFlightRemark &r) const;
};
+/// Exact-match criteria for searching previously reported remarks. Unspecified
+/// fields match any value; a specified empty string matches an empty field.
+/// Strings are owned by the query. Locations belong to their MLIRContext and
+/// must not outlive it. A location match does not imply operation identity.
+class RemarkQuery {
+public:
+ RemarkQuery name(StringRef value) const {
+ auto copy = *this;
+ copy.remarkName = value.str();
+ return copy;
+ }
+ /// Match the base category, without the subcategory.
+ RemarkQuery category(StringRef value) const {
+ auto copy = *this;
+ copy.categoryName = value.str();
+ return copy;
+ }
+ RemarkQuery subCategory(StringRef value) const {
+ auto copy = *this;
+ copy.subCategoryName = value.str();
+ return copy;
+ }
+ RemarkQuery function(StringRef value) const {
+ auto copy = *this;
+ copy.functionName = value.str();
+ return copy;
+ }
+ RemarkQuery kind(RemarkKind value) const {
+ auto copy = *this;
+ copy.remarkKind = value;
+ return copy;
+ }
+ /// Match the complete location, including any wrapping locations.
+ RemarkQuery at(Location value) const {
+ auto copy = *this;
+ copy.location = value;
+ return copy;
+ }
+
+private:
+ friend class detail::RemarkEngine;
+ std::optional<std::string> remarkName, categoryName, subCategoryName,
+ functionName;
+ std::optional<RemarkKind> remarkKind;
+ std::optional<Location> location;
+};
} // namespace mlir::remark
namespace mlir::remark::detail {
@@ -146,6 +194,7 @@ namespace mlir::remark::detail {
// Remark Base Class
//===----------------------------------------------------------------------===//
class Remark {
+ friend class RemarkEngine;
public:
Remark(RemarkKind remarkKind, DiagnosticSeverity severity, Location loc,
@@ -391,8 +440,10 @@ struct LazyTextBuild {
std::function<std::string()> thunk;
};
-/// A wrapper for linking remarks by query - searches the engine's registry
-/// at stream time and links to all matching remarks.
+/// Resolves a query when inserted into an in-flight remark.
+struct RelatedRemarkQuery {
+ RemarkQuery query;
+};
/// InFlightRemark is a RAII class that holds a reference to a Remark
/// instance and allows to build the remark using the << operator. The remark
@@ -415,9 +466,13 @@ class InFlightRemark {
return *this;
}
- // Generic path, but *not* for Lazy
- template <typename T, typename = std::enable_if_t<
- !std::is_same_v<std::decay_t<T>, LazyTextBuild>>>
+ InFlightRemark &operator<<(const RelatedRemarkQuery &related);
+
+ // Generic path, excluding wrappers handled by dedicated overloads.
+ template <typename T,
+ typename = std::enable_if_t<
+ !std::is_same_v<std::decay_t<T>, LazyTextBuild> &&
+ !std::is_same_v<std::decay_t<T>, RelatedRemarkQuery>>>
InFlightRemark &operator<<(T &&arg) {
if (remark)
*remark << std::forward<T>(arg);
@@ -480,15 +535,6 @@ class RemarkEmittingPolicyBase {
virtual void reportRemark(const Remark &remark) = 0;
virtual void finalize() = 0;
-
- /// Find previously reported remarks matching the given criteria.
- /// Default returns empty -- only policies that store remarks (like
- /// PolicyFinal) override this to enable query-based linking.
- virtual SmallVector<RemarkId>
- findRemarks(const RemarkOpts &opts,
- std::optional<RemarkKind> kind = std::nullopt) const {
- return {};
- }
};
//===----------------------------------------------------------------------===//
@@ -515,6 +561,13 @@ class RemarkEngine {
/// Atomic counter for generating unique remark IDs.
std::atomic<uint64_t> nextRemarkId{1};
+ /// Search history is independent of the emitting policy. Only metadata is
+ /// retained, and only after explicitly enabling history.
+ struct RemarkHistory;
+ std::unique_ptr<RemarkHistory> history;
+ std::atomic<bool> historyEnabled{false};
+ mutable std::mutex historyMutex;
+
/// Emit a remark using the given maker function, which should return
/// a Remark instance. The remark will be emitted using the main
/// remark streamer.
@@ -600,9 +653,28 @@ class RemarkEngine {
// Remark Linking - query previously emitted remarks
//===--------------------------------------------------------------------===//
- /// Find all remarks matching the given criteria. Delegates to the
- /// emitting policy. Only works with policies that store remarks
- /// (e.g. RemarkEmittingPolicyFinal); returns empty for PolicyAll.
+ /// Enable recording metadata for subsequent reports. Repeated enabling
+ /// preserves history. Disabling discards history. No past events are
+ /// recovered. History is retained until cleared, disabled, or the engine is
+ /// destroyed; callers reusing an engine across compilations should clear it
+ /// between runs.
+ void enableHistory(bool enable = true);
+
+ /// Discard recorded metadata without changing whether history is enabled.
+ /// Existing remark IDs and links are unaffected.
+ void clearHistory();
+
+ /// Find all recorded remarks matching every specified criterion, in report
+ /// order. Queries see a snapshot of history, including reports not yet
+ /// emitted by a deferred policy. In-flight and filtered-out remarks are not
+ /// recorded. Returns empty when history is disabled. Policy finalization does
+ /// not clear history. History access is synchronized; this does not make
+ /// emitting policies or streamers thread-safe.
+ SmallVector<RemarkId> findRemarks(const RemarkQuery &query) const;
+
+ /// Search using nonempty option fields as exact-match criteria. The related
+ /// ID in opts is ignored. Use RemarkQuery to match empty fields or a
+ /// location.
SmallVector<RemarkId>
findRemarks(const RemarkOpts &opts,
std::optional<RemarkKind> kind = std::nullopt) const;
@@ -648,6 +720,13 @@ mlir::remark::RemarkOpts::relatedTo(const detail::InFlightRemark &r) const {
}
namespace mlir::remark {
+/// Link to all remarks matching the query when this helper is inserted into an
+/// in-flight remark. History must have been enabled before reporting the target
+/// remarks. No matches (including disabled history) adds no links. Matching
+/// indicates a relationship, not causality or operation identity.
+inline detail::RelatedRemarkQuery relatedTo(RemarkQuery query) {
+ return {std::move(query)};
+}
//===----------------------------------------------------------------------===//
// Remark Emitting Policies
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index 14d095824f6712..e3f90ebf617b89 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -15,6 +15,8 @@
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
+#include <vector>
+
using namespace mlir::remark::detail;
using namespace mlir::remark;
//------------------------------------------------------------------------------
@@ -164,6 +166,13 @@ llvm::remarks::Remark Remark::generateRemark() const {
// InFlightRemark
//===----------------------------------------------------------------------===//
+InFlightRemark &InFlightRemark::operator<<(const RelatedRemarkQuery &related) {
+ if (remark && owner)
+ for (RemarkId id : owner->findRemarks(related.query))
+ remark->addRelatedRemark(id);
+ return *this;
+}
+
InFlightRemark::~InFlightRemark() {
if (remark && owner)
owner->report(std::move(*remark));
@@ -174,6 +183,71 @@ InFlightRemark::~InFlightRemark() {
// Remark Engine
//===----------------------------------------------------------------------===//
+struct RemarkEngine::RemarkHistory {
+ struct Entry {
+ RemarkId id;
+ RemarkKind kind;
+ Location location;
+ std::string name, category, subCategory, function;
+ };
+ std::vector<Entry> entries;
+};
+
+void RemarkEngine::enableHistory(bool enable) {
+ std::lock_guard<std::mutex> lock(historyMutex);
+ if (enable) {
+ if (!history)
+ history = std::make_unique<RemarkHistory>();
+ } else {
+ history.reset();
+ }
+ historyEnabled.store(enable, std::memory_order_release);
+}
+
+void RemarkEngine::clearHistory() {
+ std::lock_guard<std::mutex> lock(historyMutex);
+ if (history)
+ history->entries.clear();
+}
+
+SmallVector<RemarkId>
+RemarkEngine::findRemarks(const RemarkQuery &query) const {
+ SmallVector<RemarkId> result;
+ std::lock_guard<std::mutex> lock(historyMutex);
+ if (!history)
+ return result;
+
+ auto matches = [](const auto &filter, const auto &value) {
+ return !filter || *filter == value;
+ };
+ for (const RemarkHistory::Entry &entry : history->entries)
+ if (matches(query.remarkName, entry.name) &&
+ matches(query.categoryName, entry.category) &&
+ matches(query.subCategoryName, entry.subCategory) &&
+ matches(query.functionName, entry.function) &&
+ matches(query.remarkKind, entry.kind) &&
+ matches(query.location, entry.location))
+ result.push_back(entry.id);
+ return result;
+}
+
+SmallVector<RemarkId>
+RemarkEngine::findRemarks(const RemarkOpts &opts,
+ std::optional<RemarkKind> kind) const {
+ RemarkQuery query;
+ if (!opts.remarkName.empty())
+ query = query.name(opts.remarkName);
+ if (!opts.categoryName.empty())
+ query = query.category(opts.categoryName);
+ if (!opts.subCategoryName.empty())
+ query = query.subCategory(opts.subCategoryName);
+ if (!opts.functionName.empty())
+ query = query.function(opts.functionName);
+ if (kind)
+ query = query.kind(*kind);
+ return findRemarks(query);
+}
+
template <typename RemarkT>
InFlightRemark RemarkEngine::makeRemark(Location loc, RemarkOpts opts) {
static_assert(std::is_base_of_v<Remark, RemarkT>,
@@ -264,6 +338,15 @@ void RemarkEngine::reportImpl(const Remark &remark) {
}
void RemarkEngine::report(const Remark &&remark) {
+ if (historyEnabled.load(std::memory_order_acquire)) {
+ std::lock_guard<std::mutex> lock(historyMutex);
+ if (history && remark.getId())
+ history->entries.push_back({remark.getId(), remark.getRemarkKind(),
+ remark.getLocation(), remark.remarkName,
+ remark.categoryName, remark.subCategoryName,
+ remark.functionName});
+ }
+ // Release the history lock before callbacks, which may themselves query it.
if (remarkEmittingPolicy)
remarkEmittingPolicy->reportRemark(remark);
}
diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp
index ece76e628739fa..84cd2657792ffd 100644
--- a/mlir/unittests/IR/RemarkTest.cpp
+++ b/mlir/unittests/IR/RemarkTest.cpp
@@ -13,6 +13,7 @@
#include "mlir/IR/Remarks.h"
#include "mlir/Remark/RemarkStreamer.h"
#include "mlir/Support/TypeID.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/IR/LLVMRemarkStreamer.h"
@@ -23,6 +24,7 @@
#include "llvm/Support/YAMLParser.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include <functional>
#include <optional>
#include <vector>
@@ -795,6 +797,302 @@ TEST(Remark, TestRemarkLinking) {
EXPECT_THAT(errOut, HasSubstr("vectorized loop"));
}
+class QueryCallbackStreamer final
+ : public remark::detail::MLIRRemarkStreamerBase {
+public:
+ explicit QueryCallbackStreamer(
+ std::function<void(const remark::detail::Remark &)> callback)
+ : callback(std::move(callback)) {}
+
+ void streamOptimizationRemark(const remark::detail::Remark &remark) override {
+ callback(remark);
+ }
+
+private:
+ std::function<void(const remark::detail::Remark &)> callback;
+};
+
+class RemarkHistoryTest : public TestWithParam<bool> {
+protected:
+ struct Record {
+ remark::RemarkId id;
+ SmallVector<remark::RemarkId> related;
+ std::vector<std::string> serializedLinks;
+ };
+
+ void SetUp() override {
+ remark::RemarkCategories categories;
+ categories.passed = "(Opt.*)?";
+ categories.analysis = "(Opt.*)?";
+ std::unique_ptr<remark::detail::RemarkEmittingPolicyBase> policy;
+ if (GetParam())
+ policy = std::make_unique<remark::RemarkEmittingPolicyFinal>();
+ else
+ policy = std::make_unique<remark::RemarkEmittingPolicyAll>();
+ auto streamer = std::make_unique<QueryCallbackStreamer>([this](
+ const auto &r) {
+ Record record{r.getId(), {}, {}};
+ llvm::append_range(record.related, r.getRelatedRemarkIds());
+ for (const auto &arg : r.generateRemark().Args)
+ if (arg.Key == "RelatedTo")
+ record.serializedLinks.push_back(arg.Val.str());
+ records.push_back(std::move(record));
+ if (queryInCallback)
+ callbackHistory.push_back(engine().findRemarks(remark::RemarkQuery{}));
+ });
+ ASSERT_TRUE(succeeded(remark::enableOptimizationRemarks(
+ context, std::move(streamer), std::move(policy), categories)));
+ ASSERT_TRUE(engine().isPassedOptRemarkEnabled("Opt"));
+ ASSERT_TRUE(engine().isAnalysisOptRemarkEnabled("Opt"));
+ ASSERT_TRUE(engine().isPassedOptRemarkEnabled(""));
+ ASSERT_TRUE(engine().isAnalysisOptRemarkEnabled(""));
+ }
+
+ remark::detail::RemarkEngine &engine() { return *context.getRemarkEngine(); }
+
+ remark::RemarkId emit(Location loc, remark::RemarkOpts opts,
+ bool analysis = false) {
+ auto r = analysis ? remark::analysis(loc, opts) : remark::passed(loc, opts);
+ return r.getId();
+ }
+
+ void flush() { engine().getRemarkEmittingPolicy()->finalize(); }
+
+ std::vector<Record> records;
+ bool queryInCallback = false;
+ std::vector<SmallVector<remark::RemarkId>> callbackHistory;
+ // Keep callback state alive while the context drains its remark engine.
+ MLIRContext context;
+};
+
+TEST_P(RemarkHistoryTest, LifecycleAndReportOrder) {
+ Location loc = UnknownLoc::get(&context);
+ auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
+ auto any = remark::RemarkQuery{};
+ emit(loc, opts);
+ EXPECT_THAT(engine().findRemarks(any), IsEmpty());
+
+ engine().enableHistory();
+ remark::RemarkId firstCreated, secondCreated;
+ {
+ auto pending = remark::passed(loc, opts);
+ firstCreated = pending.getId();
+ EXPECT_THAT(engine().findRemarks(any), IsEmpty());
+ secondCreated = emit(loc, opts);
+ EXPECT_THAT(engine().findRemarks(any), ElementsAre(secondCreated));
+ }
+ EXPECT_LT(firstCreated.getValue(), secondCreated.getValue());
+ EXPECT_THAT(engine().findRemarks(any),
+ ElementsAre(secondCreated, firstCreated));
+
+ // FINAL may discard an earlier report, but history keeps both events.
+ flush();
+ engine().enableHistory();
+ EXPECT_THAT(engine().findRemarks(any),
+ ElementsAre(secondCreated, firstCreated));
+ engine().clearHistory();
+ EXPECT_THAT(engine().findRemarks(any), IsEmpty());
+ auto afterClear = emit(loc, opts);
+ EXPECT_THAT(engine().findRemarks(any), ElementsAre(afterClear));
+ EXPECT_GT(afterClear.getValue(), secondCreated.getValue());
+
+ engine().enableHistory(false);
+ EXPECT_THAT(engine().findRemarks(any), IsEmpty());
+ emit(loc, opts);
+ engine().clearHistory();
+ emit(loc, opts);
+ EXPECT_THAT(engine().findRemarks(any), IsEmpty());
+ engine().enableHistory();
+ EXPECT_THAT(engine().findRemarks(any), IsEmpty());
+ auto afterEnable = emit(loc, opts);
+ EXPECT_THAT(engine().findRemarks(any), ElementsAre(afterEnable));
+ EXPECT_GT(afterEnable.getValue(), afterClear.getValue());
+
+ EXPECT_FALSE(static_cast<bool>(
+ emit(loc, remark::RemarkOpts::name("Filtered").category("Unselected"))));
+ EXPECT_THAT(engine().findRemarks(any), ElementsAre(afterEnable));
+}
+
+TEST_P(RemarkHistoryTest, ExactFieldsAndConjunction) {
+ engine().enableHistory();
+ Location loc = FileLineColLoc::get(&context, "input.mlir", 7, 2);
+ Location otherLoc = NameLoc::get(StringAttr::get(&context, "wrapped"), loc);
+ auto opts = remark::RemarkOpts::name("Candidate")
+ .category("Opt")
+ .subCategory("Vector")
+ .function("kernel");
+ auto target = emit(loc, opts);
+ auto otherSub = emit(loc, opts.subCategory("Unroll"));
+ auto analysis = emit(loc, opts, /*analysis=*/true);
+ auto other = emit(otherLoc, remark::RemarkOpts::name("Other")
+ .category("OptOther")
+ .subCategory("Other")
+ .function("helper"));
+ auto empty = emit(UnknownLoc::get(&context), remark::RemarkOpts{});
+ auto any = remark::RemarkQuery{};
+
+ EXPECT_THAT(engine().findRemarks(any.name("Candidate")),
+ ElementsAre(target, otherSub, analysis));
+ EXPECT_THAT(engine().findRemarks(any.category("Opt")),
+ ElementsAre(target, otherSub, analysis));
+ EXPECT_THAT(engine().findRemarks(any.subCategory("Vector")),
+ ElementsAre(target, analysis));
+ EXPECT_THAT(engine().findRemarks(any.function("kernel")),
+ ElementsAre(target, otherSub, analysis));
+ EXPECT_THAT(engine().findRemarks(any.kind(remark::RemarkKind::RemarkPassed)),
+ ElementsAre(target, otherSub, other, empty));
+ EXPECT_THAT(engine().findRemarks(any.at(loc)),
+ ElementsAre(target, otherSub, analysis));
+ EXPECT_THAT(engine().findRemarks(any.at(otherLoc)), ElementsAre(other));
+
+ auto exact = any.name("Candidate")
+ .category("Opt")
+ .subCategory("Vector")
+ .function("kernel")
+ .kind(remark::RemarkKind::RemarkPassed)
+ .at(loc);
+ EXPECT_THAT(engine().findRemarks(exact), ElementsAre(target));
+ EXPECT_THAT(engine().findRemarks(exact.function("helper")), IsEmpty());
+ EXPECT_THAT(engine().findRemarks(any.category("Opt.*")), IsEmpty());
+ EXPECT_THAT(engine().findRemarks(any.category("Opt:Vector")), IsEmpty());
+ EXPECT_THAT(engine().findRemarks(any.name("")), ElementsAre(empty));
+ EXPECT_THAT(engine().findRemarks(any.category("")), ElementsAre(empty));
+ EXPECT_THAT(engine().findRemarks(any.subCategory("")), ElementsAre(empty));
+ EXPECT_THAT(engine().findRemarks(any.function("")), ElementsAre(empty));
+ EXPECT_THAT(engine().findRemarks(any.name("<unknown remark name>")),
+ IsEmpty());
+ EXPECT_THAT(engine().findRemarks(any.function("<unknown function>")),
+ IsEmpty());
+
+ // The existing options overload treats empty fields as wildcards.
+ EXPECT_THAT(engine().findRemarks(remark::RemarkOpts{}),
+ ElementsAre(target, otherSub, analysis, other, empty));
+ EXPECT_THAT(engine().findRemarks(opts, remark::RemarkKind::RemarkAnalysis),
+ ElementsAre(analysis));
+ EXPECT_THAT(engine().findRemarks(opts.relatedTo(other)),
+ ElementsAre(target, analysis));
+}
+
+TEST_P(RemarkHistoryTest, OwnsQueryAndHistoryStrings) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ remark::RemarkQuery query;
+ remark::RemarkId target;
+ {
+ std::string name = "Candidate", category = "Opt", sub = "Vector";
+ std::string function = "kernel";
+ query =
+ query.name(name).category(category).subCategory(sub).function(function);
+ target = emit(loc, remark::RemarkOpts::name(name)
+ .category(category)
+ .subCategory(sub)
+ .function(function));
+ name.assign("Changed");
+ category.assign("Changed");
+ sub.assign("Changed");
+ function.assign("Changed");
+ }
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(target));
+ const auto original = query;
+ auto changed = original.name("Other");
+ EXPECT_THAT(engine().findRemarks(changed), IsEmpty());
+ EXPECT_THAT(engine().findRemarks(original), ElementsAre(target));
+}
+
+TEST_P(RemarkHistoryTest, QueryLinksSnapshotAndSerialization) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto query = remark::RemarkQuery{}.kind(remark::RemarkKind::RemarkAnalysis);
+ auto first =
+ emit(loc, remark::RemarkOpts::name("First").category("Opt"), true);
+ auto second =
+ emit(loc, remark::RemarkOpts::name("Second").category("Opt"), true);
+ remark::RemarkId linked, later, pendingId, noMatches, copiedWrapper;
+ {
+ auto pending = remark::analysis(
+ loc, remark::RemarkOpts::name("Pending").category("Opt"));
+ pendingId = pending.getId();
+ {
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("Linked").category("Opt"));
+ linked = r.getId();
+ r << remark::relatedTo(query);
+ // This report occurs before Linked is reported, but after its query.
+ later =
+ emit(loc, remark::RemarkOpts::name("Later").category("Opt"), true);
+ }
+ }
+ {
+ const auto wrapper = remark::relatedTo(query);
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("CopiedWrapper").category("Opt"));
+ copiedWrapper = r.getId();
+ r << wrapper;
+ }
+ {
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("NoMatches").category("Opt"));
+ noMatches = r.getId();
+ r << remark::relatedTo(query.name("Absent"));
+ }
+ flush();
+
+ for (const auto &record : records) {
+ if (record.id == linked) {
+ EXPECT_THAT(record.related, ElementsAre(first, second));
+ EXPECT_THAT(record.serializedLinks,
+ ElementsAre(std::to_string(first.getValue()),
+ std::to_string(second.getValue())));
+ } else if (record.id == copiedWrapper) {
+ EXPECT_THAT(record.related, ElementsAre(first, second, later, pendingId));
+ } else if (record.id == noMatches) {
+ EXPECT_THAT(record.related, IsEmpty());
+ EXPECT_THAT(record.serializedLinks, IsEmpty());
+ }
+ }
+ EXPECT_THAT(records, Contains(Field(&Record::id, linked)));
+ EXPECT_THAT(records, Contains(Field(&Record::id, copiedWrapper)));
+ EXPECT_THAT(records, Contains(Field(&Record::id, noMatches)));
+}
+
+TEST_P(RemarkHistoryTest, QueryLinksDoNotEnableHistory) {
+ Location loc = UnknownLoc::get(&context);
+ auto query = remark::RemarkQuery{};
+ emit(loc, remark::RemarkOpts::name("First").category("Opt"));
+ remark::RemarkId linked;
+ {
+ auto r =
+ remark::passed(loc, remark::RemarkOpts::name("Linked").category("Opt"));
+ linked = r.getId();
+ auto wrapper = remark::relatedTo(query);
+ r << wrapper;
+ }
+ // Both filtered and ownerless remarks accept the lazy wrapper harmlessly.
+ remark::passed(loc, remark::RemarkOpts::name("Filtered").category("Other"))
+ << remark::relatedTo(query);
+ remark::detail::InFlightRemark{} << remark::relatedTo(query);
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+ engine().enableHistory();
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+ flush();
+ EXPECT_THAT(records, Contains(AllOf(Field(&Record::id, linked),
+ Field(&Record::related, IsEmpty()))));
+}
+
+TEST_P(RemarkHistoryTest, StreamerCanQueryCurrentReport) {
+ engine().enableHistory();
+ queryInCallback = true;
+ auto id = emit(UnknownLoc::get(&context),
+ remark::RemarkOpts::name("Candidate").category("Opt"));
+ flush();
+ EXPECT_THAT(callbackHistory, ElementsAre(ElementsAre(id)));
+}
+
+INSTANTIATE_TEST_SUITE_P(EmittingPolicies, RemarkHistoryTest, Bool(),
+ [](const TestParamInfo<bool> &info) {
+ return info.param ? "Final" : "All";
+ });
+
TEST(Remark, TestNestedLocation) {
MLIRContext context;
Location fileLoc = FileLineColLoc::get(&context, "test.mlir", 42, 7);
>From 231085ff624f9e4fd0319e92a3d396d7d64f8e93 Mon Sep 17 00:00:00 2001
From: Guray Ozen <gozen at nvidia.com>
Date: Tue, 3 Nov 2026 10:49:52 +0100
Subject: [PATCH 2/4] [mlir] Record remark history after policy selection
---
mlir/docs/Remarks.md | 100 ++++-------
mlir/include/mlir/IR/Remarks.h | 134 +++++++++-----
mlir/lib/IR/Remarks.cpp | 121 +++++++++----
mlir/unittests/IR/RemarkTest.cpp | 291 +++++++++++++++++++++++++++++--
4 files changed, 494 insertions(+), 152 deletions(-)
diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md
index 663eb1161e9ad8..0ff919588f08bc 100644
--- a/mlir/docs/Remarks.md
+++ b/mlir/docs/Remarks.md
@@ -253,86 +253,62 @@ if (engine->isRemarkEnabled(remark::RemarkKind::RemarkPassed, "Vectorizer"))
## Searching and Linking Remarks
-Remarks can refer to other remarks by ID using `RemarkOpts::relatedTo(id)`.
-When the producer does not have the ID, it can search previously reported
-remarks and link to the matches.
-
-First, enable history on an already configured engine:
+History is opt-in and records each emitted event ID once, in first-emission
+order, before the streamer callback. It retains IDs and identifying metadata,
+with interned strings, but not messages or arguments. `All` delivers reports
+immediately; `Final` makes them searchable only when finalized and excludes
+discarded events. Previously emitted targets remain searchable across batches.
+Enable history before emission: pending `Final` remarks can still be recorded
+when delivered, but earlier emissions and engine-filtered remarks are not recovered.
+
+`RemarkQuery` matches name, base category, subcategory, function, kind, and
+complete location, including wrappers. Specified fields use exact AND matching;
+unspecified fields match anything, and an empty string matches empty metadata.
+For example, with an already configured `All` engine and enabled categories:
```c++
auto *engine = context.getRemarkEngine();
engine->enableHistory();
-```
-
-History is disabled by default. Enabling it retains the IDs and identifying
-metadata of subsequent reports, without retaining their messages or arguments.
-It does not recover earlier reports or remarks excluded by the engine's
-category and kind filters. An in-flight remark becomes searchable when it is
-reported at the end of its lifetime.
-
-Use `RemarkQuery` to match a name, base category, subcategory, function, kind,
-or location. Specified fields are combined with AND and match exactly, without
-regular expressions. Unspecified fields match anything; an explicitly empty
-string matches empty metadata. Location matching compares the complete MLIR
-location, including wrappers, and does not identify a unique operation.
-
-```c++
auto analysisOpts = remark::RemarkOpts::name("VectorSizeInference")
.category("Vectorizer")
.function("kernel");
remark::analysis(loc, analysisOpts)
<< remark::reason("Unable to infer vector sizes");
-auto query = remark::RemarkQuery()
- .name("VectorSizeInference")
+auto query = remark::RemarkQuery::named("VectorSizeInference")
.category("Vectorizer")
.function("kernel")
.kind(remark::RemarkKind::RemarkAnalysis)
.at(loc);
-
-// Obtain the IDs explicitly, in report order.
+// Matching IDs are returned in first-emission order.
SmallVector<remark::RemarkId> matches = engine->findRemarks(query);
-
-// Or link all matching reports without handling their IDs.
remark::missed(loc, remark::RemarkOpts::name("Vectorization")
- .category("Vectorizer")
- .function("kernel"))
- << remark::reason("Cannot vectorize without vector sizes")
- << remark::relatedTo(query);
-```
-
-The helper resolves the query when inserted with `<<`. Later reports do not
-change the links. No matches, including when history is disabled, adds no
-links. Links use the existing repeated `RelatedTo` arguments in serialized
-output. Consumers must preserve repeated keys. A query match indicates a
-relationship, not causality; the producer is responsible for choosing relevant
-criteria. There is no automatic selection of the latest match or inference of
-whether earlier evidence is still valid for changed IR.
-
-Queries own their strings, so they can be retained and reused. A query with a
-location must not outlive that location's `MLIRContext`. History access is
-synchronized, and streamer callbacks may query the history, including the
-report being delivered. This does not synchronize the emitting policy or
-streamer. Report order follows insertion into history and can differ from ID
-creation order, including when remarks are emitted concurrently.
-
-History is independent of the output policy: both `All` and `Final` record
-every report, and finalizing a policy does not clear history. Use `All` when
-the output must contain every referenced event. `Final` may deduplicate a
-referenced report or emit it in an earlier batch, so finding its ID in history
-does not guarantee that the current output batch contains it.
-
-History grows with the number of reports until explicitly cleared, disabled,
-or the engine is destroyed. Clear it between compilations when reusing an
-engine:
-
-```c++
-engine->clearHistory(); // Keep recording subsequent reports.
-engine->enableHistory(false); // Discard history and stop recording.
+ .category("Vectorizer")
+ .function("kernel"))
+ << remark::relatedToLatest(query);
```
-Neither operation resets the ID counter or changes existing links. Repeated
-calls to `enableHistory()` preserve already recorded history.
+`relatedTo(query)` links all matches; `relatedToLatest(query)` links at most one,
+the most recent matching first-emission entry. Both resolve at `<<`; no match
+or disabled history adds no links, and later emissions do not change them.
+Known IDs can use `RemarkOpts::relatedTo(id)`. Serialized links repeat the
+`RelatedTo` key, so consumers must preserve duplicates. Matching does not establish
+causality, operation identity, or continued validity after IR changes.
+
+Queries own their criteria strings; queries with locations must not outlive the
+locations' `MLIRContext`.
+History access is synchronized, including queries from streamer callbacks that
+see the current event. This does not synchronize policies or streamers. With a
+parallel pass manager, cross-function query membership and ordering depend on
+timing; scope queries to a function and sequence producers or use explicit
+barriers. First-emission order can differ from ID creation order.
+
+Memory is retained until `clearHistory()`, `enableHistory(false)`, or engine
+destruction. Clearing keeps recording; disabling stops it. Neither resets IDs,
+changes existing links, nor cancels pending `Final` remarks. Repeated enabling
+preserves entries. Latest-match search scans backward and stops at its first
+match, but remains O(N) in the worst case. Repeated broad all-match queries can
+produce quadratic numbers of links.
***
diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 6acbc722944ff4..49c5e1ff5abd70 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -13,6 +13,7 @@
#ifndef MLIR_IR_REMARKS_H
#define MLIR_IR_REMARKS_H
+#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/Remarks/Remark.h"
@@ -24,7 +25,7 @@
#include "mlir/IR/Value.h"
#include <atomic>
-#include <mutex>
+#include <utility>
namespace mlir::remark {
@@ -141,43 +142,60 @@ struct RemarkOpts {
inline RemarkOpts relatedTo(const detail::InFlightRemark &r) const;
};
-/// Exact-match criteria for searching previously reported remarks. Unspecified
+/// Exact-match criteria for searching previously emitted remarks. Unspecified
/// fields match any value; a specified empty string matches an empty field.
/// Strings are owned by the query. Locations belong to their MLIRContext and
/// must not outlive it. A location match does not imply operation identity.
class RemarkQuery {
public:
- RemarkQuery name(StringRef value) const {
- auto copy = *this;
- copy.remarkName = value.str();
- return copy;
+ /// Construct a query matching the given remark name.
+ [[nodiscard]] static RemarkQuery named(StringRef value) {
+ return RemarkQuery().name(value);
+ }
+
+ [[nodiscard]] RemarkQuery name(StringRef value) const & {
+ return RemarkQuery(*this).name(value);
+ }
+ [[nodiscard]] RemarkQuery name(StringRef value) && {
+ remarkName = value.str();
+ return std::move(*this);
}
/// Match the base category, without the subcategory.
- RemarkQuery category(StringRef value) const {
- auto copy = *this;
- copy.categoryName = value.str();
- return copy;
+ [[nodiscard]] RemarkQuery category(StringRef value) const & {
+ return RemarkQuery(*this).category(value);
}
- RemarkQuery subCategory(StringRef value) const {
- auto copy = *this;
- copy.subCategoryName = value.str();
- return copy;
+ [[nodiscard]] RemarkQuery category(StringRef value) && {
+ categoryName = value.str();
+ return std::move(*this);
}
- RemarkQuery function(StringRef value) const {
- auto copy = *this;
- copy.functionName = value.str();
- return copy;
+ [[nodiscard]] RemarkQuery subCategory(StringRef value) const & {
+ return RemarkQuery(*this).subCategory(value);
}
- RemarkQuery kind(RemarkKind value) const {
- auto copy = *this;
- copy.remarkKind = value;
- return copy;
+ [[nodiscard]] RemarkQuery subCategory(StringRef value) && {
+ subCategoryName = value.str();
+ return std::move(*this);
+ }
+ [[nodiscard]] RemarkQuery function(StringRef value) const & {
+ return RemarkQuery(*this).function(value);
+ }
+ [[nodiscard]] RemarkQuery function(StringRef value) && {
+ functionName = value.str();
+ return std::move(*this);
+ }
+ [[nodiscard]] RemarkQuery kind(RemarkKind value) const & {
+ return RemarkQuery(*this).kind(value);
+ }
+ [[nodiscard]] RemarkQuery kind(RemarkKind value) && {
+ remarkKind = value;
+ return std::move(*this);
}
/// Match the complete location, including any wrapping locations.
- RemarkQuery at(Location value) const {
- auto copy = *this;
- copy.location = value;
- return copy;
+ [[nodiscard]] RemarkQuery at(Location value) const & {
+ return RemarkQuery(*this).at(value);
+ }
+ [[nodiscard]] RemarkQuery at(Location value) && {
+ location = value;
+ return std::move(*this);
}
private:
@@ -194,8 +212,6 @@ namespace mlir::remark::detail {
// Remark Base Class
//===----------------------------------------------------------------------===//
class Remark {
- friend class RemarkEngine;
-
public:
Remark(RemarkKind remarkKind, DiagnosticSeverity severity, Location loc,
RemarkOpts opts)
@@ -267,8 +283,13 @@ class Remark {
return "<unknown function>";
}
+ /// Return the stored function name, which may be empty.
+ StringRef getRawFunction() const { return functionName; }
+
llvm::StringRef getCategoryName() const { return categoryName; }
+ llvm::StringRef getSubCategoryName() const { return subCategoryName; }
+
llvm::StringRef getCombinedCategoryName() const {
if (categoryName.empty() && subCategoryName.empty())
return {};
@@ -285,6 +306,9 @@ class Remark {
return remarkName;
}
+ /// Return the stored remark name, which may be empty.
+ StringRef getRawRemarkName() const { return remarkName; }
+
std::string getMsg() const;
ArrayRef<Arg> getArgs() const { return args; }
@@ -443,6 +467,7 @@ struct LazyTextBuild {
/// Resolves a query when inserted into an in-flight remark.
struct RelatedRemarkQuery {
RemarkQuery query;
+ bool latestOnly = false;
};
/// InFlightRemark is a RAII class that holds a reference to a Remark
@@ -535,6 +560,14 @@ class RemarkEmittingPolicyBase {
virtual void reportRemark(const Remark &remark) = 0;
virtual void finalize() = 0;
+
+ /// Legacy policy-specific lookup hook. Engine history queries use the
+ /// records actually emitted by the policy, independently of this hook.
+ virtual SmallVector<RemarkId>
+ findRemarks(const RemarkOpts &opts,
+ std::optional<RemarkKind> kind = std::nullopt) const {
+ return {};
+ }
};
//===----------------------------------------------------------------------===//
@@ -543,6 +576,8 @@ class RemarkEmittingPolicyBase {
class RemarkEngine {
private:
+ friend class InFlightRemark;
+
/// Regex that filters missed optimization remarks: only matching one are
/// reported.
std::optional<llvm::Regex> missFilter;
@@ -561,12 +596,17 @@ class RemarkEngine {
/// Atomic counter for generating unique remark IDs.
std::atomic<uint64_t> nextRemarkId{1};
- /// Search history is independent of the emitting policy. Only metadata is
- /// retained, and only after explicitly enabling history.
+ /// History records metadata for remarks selected for emission by the policy.
+ /// Its mutex has a stable lifetime; retained storage is allocated on demand.
struct RemarkHistory;
std::unique_ptr<RemarkHistory> history;
std::atomic<bool> historyEnabled{false};
- mutable std::mutex historyMutex;
+
+ /// Visit matches while holding the history lock. Only internal callbacks that
+ /// do not reenter the engine may be used here.
+ void forEachMatch(const RemarkQuery &query,
+ llvm::function_ref<void(RemarkId)> callback,
+ bool latestOnly = false) const;
/// Emit a remark using the given maker function, which should return
/// a Remark instance. The remark will be emitted using the main
@@ -653,8 +693,8 @@ class RemarkEngine {
// Remark Linking - query previously emitted remarks
//===--------------------------------------------------------------------===//
- /// Enable recording metadata for subsequent reports. Repeated enabling
- /// preserves history. Disabling discards history. No past events are
+ /// Enable recording metadata for subsequent policy emissions. Repeated
+ /// enabling preserves history. Disabling discards history. No past events are
/// recovered. History is retained until cleared, disabled, or the engine is
/// destroyed; callers reusing an engine across compilations should clear it
/// between runs.
@@ -664,12 +704,17 @@ class RemarkEngine {
/// Existing remark IDs and links are unaffected.
void clearHistory();
- /// Find all recorded remarks matching every specified criterion, in report
- /// order. Queries see a snapshot of history, including reports not yet
- /// emitted by a deferred policy. In-flight and filtered-out remarks are not
- /// recorded. Returns empty when history is disabled. Policy finalization does
- /// not clear history. History access is synchronized; this does not make
- /// emitting policies or streamers thread-safe.
+ /// Find distinct emitted remarks matching every specified criterion, in
+ /// first-emission order. Pending, engine-filtered and policy-discarded
+ /// remarks are absent. Returns empty when history is disabled. Policy
+ /// finalization may add entries but does not clear history. A callback can
+ /// query the event currently being delivered. History access is synchronized;
+ /// this does not make emitting policies or streamers thread-safe.
+ ///
+ /// Cross-function queries during parallel pass execution have
+ /// timing-dependent results and ordering. Scope queries with function() and
+ /// sequence producers before consumers when deterministic relationships are
+ /// required.
SmallVector<RemarkId> findRemarks(const RemarkQuery &query) const;
/// Search using nonempty option fields as exact-match criteria. The related
@@ -721,13 +766,20 @@ mlir::remark::RemarkOpts::relatedTo(const detail::InFlightRemark &r) const {
namespace mlir::remark {
/// Link to all remarks matching the query when this helper is inserted into an
-/// in-flight remark. History must have been enabled before reporting the target
-/// remarks. No matches (including disabled history) adds no links. Matching
+/// in-flight remark. History must have been enabled before the policy emitted
+/// the remarks. No matches (including disabled history) adds no links. Matching
/// indicates a relationship, not causality or operation identity.
inline detail::RelatedRemarkQuery relatedTo(RemarkQuery query) {
return {std::move(query)};
}
+/// Link only the latest match in first-emission order. The reverse search stops
+/// at the first match and adds at most one RelatedTo argument. Repeated
+/// delivery of an existing event does not refresh its position in history.
+inline detail::RelatedRemarkQuery relatedToLatest(RemarkQuery query) {
+ return {std::move(query), true};
+}
+
//===----------------------------------------------------------------------===//
// Remark Emitting Policies
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index e3f90ebf617b89..d912d35020e31f 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -14,7 +14,9 @@
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/StringSaver.h"
+#include <mutex>
#include <vector>
using namespace mlir::remark::detail;
@@ -168,8 +170,9 @@ llvm::remarks::Remark Remark::generateRemark() const {
InFlightRemark &InFlightRemark::operator<<(const RelatedRemarkQuery &related) {
if (remark && owner)
- for (RemarkId id : owner->findRemarks(related.query))
- remark->addRelatedRemark(id);
+ owner->forEachMatch(
+ related.query, [this](RemarkId id) { remark->addRelatedRemark(id); },
+ related.latestOnly);
return *this;
}
@@ -188,46 +191,90 @@ struct RemarkEngine::RemarkHistory {
RemarkId id;
RemarkKind kind;
Location location;
- std::string name, category, subCategory, function;
+ StringRef name, category, subCategory, function;
+
+ static Entry from(const Remark &remark, llvm::UniqueStringSaver &strings) {
+ return {remark.getId(),
+ remark.getRemarkKind(),
+ remark.getLocation(),
+ strings.save(remark.getRawRemarkName()),
+ strings.save(remark.getCategoryName()),
+ strings.save(remark.getSubCategoryName()),
+ strings.save(remark.getRawFunction())};
+ }
+ };
+
+ struct Storage {
+ llvm::BumpPtrAllocator allocator;
+ llvm::UniqueStringSaver strings{allocator};
+ std::vector<Entry> entries;
+ llvm::DenseSet<uint64_t> emittedIds;
};
- std::vector<Entry> entries;
+
+ // Keep the mutex alive for the engine's lifetime so disabling history cannot
+ // destroy it while another thread is about to lock it. Storage is opt-in.
+ std::mutex mutex;
+ std::unique_ptr<Storage> storage;
};
void RemarkEngine::enableHistory(bool enable) {
- std::lock_guard<std::mutex> lock(historyMutex);
+ std::scoped_lock lock(history->mutex);
if (enable) {
- if (!history)
- history = std::make_unique<RemarkHistory>();
+ if (!history->storage)
+ history->storage = std::make_unique<RemarkHistory::Storage>();
} else {
- history.reset();
+ history->storage.reset();
}
historyEnabled.store(enable, std::memory_order_release);
}
void RemarkEngine::clearHistory() {
- std::lock_guard<std::mutex> lock(historyMutex);
- if (history)
- history->entries.clear();
+ if (!historyEnabled.load(std::memory_order_acquire))
+ return;
+ std::scoped_lock lock(history->mutex);
+ if (history->storage)
+ history->storage = std::make_unique<RemarkHistory::Storage>();
}
-SmallVector<RemarkId>
-RemarkEngine::findRemarks(const RemarkQuery &query) const {
- SmallVector<RemarkId> result;
- std::lock_guard<std::mutex> lock(historyMutex);
- if (!history)
- return result;
+void RemarkEngine::forEachMatch(const RemarkQuery &query,
+ llvm::function_ref<void(RemarkId)> callback,
+ bool latestOnly) const {
+ if (!historyEnabled.load(std::memory_order_acquire))
+ return;
+ std::scoped_lock lock(history->mutex);
+ if (!history->storage)
+ return;
auto matches = [](const auto &filter, const auto &value) {
return !filter || *filter == value;
};
- for (const RemarkHistory::Entry &entry : history->entries)
- if (matches(query.remarkName, entry.name) &&
- matches(query.categoryName, entry.category) &&
- matches(query.subCategoryName, entry.subCategory) &&
- matches(query.functionName, entry.function) &&
- matches(query.remarkKind, entry.kind) &&
- matches(query.location, entry.location))
- result.push_back(entry.id);
+ auto matchesQuery = [&](const RemarkHistory::Entry &entry) {
+ return matches(query.remarkName, entry.name) &&
+ matches(query.categoryName, entry.category) &&
+ matches(query.subCategoryName, entry.subCategory) &&
+ matches(query.functionName, entry.function) &&
+ matches(query.remarkKind, entry.kind) &&
+ matches(query.location, entry.location);
+ };
+ const auto &entries = history->storage->entries;
+ if (latestOnly) {
+ for (const RemarkHistory::Entry &entry : llvm::reverse(entries)) {
+ if (!matchesQuery(entry))
+ continue;
+ callback(entry.id);
+ break;
+ }
+ return;
+ }
+ for (const RemarkHistory::Entry &entry : entries)
+ if (matchesQuery(entry))
+ callback(entry.id);
+}
+
+SmallVector<RemarkId>
+RemarkEngine::findRemarks(const RemarkQuery &query) const {
+ SmallVector<RemarkId> result;
+ forEachMatch(query, [&](RemarkId id) { result.push_back(id); });
return result;
}
@@ -327,6 +374,18 @@ InFlightRemark RemarkEngine::emitOptimizationRemarkAnalysis(Location loc,
//===----------------------------------------------------------------------===//
void RemarkEngine::reportImpl(const Remark &remark) {
+ if (historyEnabled.load(std::memory_order_acquire)) {
+ std::scoped_lock lock(history->mutex);
+ if (history->storage && remark.getId()) {
+ auto &storage = *history->storage;
+ // FINAL can emit a shared related remark more than once. Keep one history
+ // entry per event so a query does not produce duplicate links.
+ if (storage.emittedIds.insert(remark.getId().getValue()).second)
+ storage.entries.push_back(
+ RemarkHistory::Entry::from(remark, storage.strings));
+ }
+ }
+ // Release the history lock before callbacks, which may themselves query it.
// Stream the remark
if (remarkStreamer) {
remarkStreamer->streamOptimizationRemark(remark);
@@ -338,15 +397,6 @@ void RemarkEngine::reportImpl(const Remark &remark) {
}
void RemarkEngine::report(const Remark &&remark) {
- if (historyEnabled.load(std::memory_order_acquire)) {
- std::lock_guard<std::mutex> lock(historyMutex);
- if (history && remark.getId())
- history->entries.push_back({remark.getId(), remark.getRemarkKind(),
- remark.getLocation(), remark.remarkName,
- remark.categoryName, remark.subCategoryName,
- remark.functionName});
- }
- // Release the history lock before callbacks, which may themselves query it.
if (remarkEmittingPolicy)
remarkEmittingPolicy->reportRemark(remark);
}
@@ -413,7 +463,8 @@ buildFilter(const mlir::remark::RemarkCategories &cats,
RemarkEngine::RemarkEngine(bool printAsEmitRemarks,
const RemarkCategories &cats)
- : printAsEmitRemarks(printAsEmitRemarks) {
+ : printAsEmitRemarks(printAsEmitRemarks),
+ history(std::make_unique<RemarkHistory>()) {
if (cats.passed)
passedFilter = buildFilter(cats, cats.passed);
if (cats.missed)
diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp
index 84cd2657792ffd..9b1c4240165411 100644
--- a/mlir/unittests/IR/RemarkTest.cpp
+++ b/mlir/unittests/IR/RemarkTest.cpp
@@ -858,6 +858,17 @@ class RemarkHistoryTest : public TestWithParam<bool> {
void flush() { engine().getRemarkEmittingPolicy()->finalize(); }
+ void expectLinks(remark::RemarkId id, ArrayRef<remark::RemarkId> expected) {
+ std::vector<std::string> serialized;
+ for (remark::RemarkId related : expected)
+ serialized.push_back(std::to_string(related.getValue()));
+ EXPECT_THAT(records, Contains(AllOf(Field(&Record::id, id),
+ Field(&Record::related,
+ ElementsAreArray(expected)),
+ Field(&Record::serializedLinks,
+ ElementsAreArray(serialized)))));
+ }
+
std::vector<Record> records;
bool queryInCallback = false;
std::vector<SmallVector<remark::RemarkId>> callbackHistory;
@@ -865,11 +876,12 @@ class RemarkHistoryTest : public TestWithParam<bool> {
MLIRContext context;
};
-TEST_P(RemarkHistoryTest, LifecycleAndReportOrder) {
+TEST_P(RemarkHistoryTest, LifecycleAndEmissionOrder) {
Location loc = UnknownLoc::get(&context);
auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
auto any = remark::RemarkQuery{};
emit(loc, opts);
+ flush();
EXPECT_THAT(engine().findRemarks(any), IsEmpty());
engine().enableHistory();
@@ -878,33 +890,42 @@ TEST_P(RemarkHistoryTest, LifecycleAndReportOrder) {
auto pending = remark::passed(loc, opts);
firstCreated = pending.getId();
EXPECT_THAT(engine().findRemarks(any), IsEmpty());
- secondCreated = emit(loc, opts);
- EXPECT_THAT(engine().findRemarks(any), ElementsAre(secondCreated));
+ secondCreated =
+ emit(loc, remark::RemarkOpts::name("Second").category("Opt"));
+ if (GetParam())
+ EXPECT_THAT(engine().findRemarks(any), IsEmpty());
+ else
+ EXPECT_THAT(engine().findRemarks(any), ElementsAre(secondCreated));
}
EXPECT_LT(firstCreated.getValue(), secondCreated.getValue());
- EXPECT_THAT(engine().findRemarks(any),
- ElementsAre(secondCreated, firstCreated));
-
- // FINAL may discard an earlier report, but history keeps both events.
+ // ALL emits immediately; FINAL emits these independent records in ID order.
flush();
engine().enableHistory();
- EXPECT_THAT(engine().findRemarks(any),
- ElementsAre(secondCreated, firstCreated));
+ if (GetParam())
+ EXPECT_THAT(engine().findRemarks(any),
+ ElementsAre(firstCreated, secondCreated));
+ else
+ EXPECT_THAT(engine().findRemarks(any),
+ ElementsAre(secondCreated, firstCreated));
engine().clearHistory();
EXPECT_THAT(engine().findRemarks(any), IsEmpty());
auto afterClear = emit(loc, opts);
+ flush();
EXPECT_THAT(engine().findRemarks(any), ElementsAre(afterClear));
EXPECT_GT(afterClear.getValue(), secondCreated.getValue());
engine().enableHistory(false);
EXPECT_THAT(engine().findRemarks(any), IsEmpty());
emit(loc, opts);
+ flush();
engine().clearHistory();
emit(loc, opts);
+ flush();
EXPECT_THAT(engine().findRemarks(any), IsEmpty());
engine().enableHistory();
EXPECT_THAT(engine().findRemarks(any), IsEmpty());
auto afterEnable = emit(loc, opts);
+ flush();
EXPECT_THAT(engine().findRemarks(any), ElementsAre(afterEnable));
EXPECT_GT(afterEnable.getValue(), afterClear.getValue());
@@ -930,6 +951,7 @@ TEST_P(RemarkHistoryTest, ExactFieldsAndConjunction) {
.function("helper"));
auto empty = emit(UnknownLoc::get(&context), remark::RemarkOpts{});
auto any = remark::RemarkQuery{};
+ flush();
EXPECT_THAT(engine().findRemarks(any.name("Candidate")),
ElementsAre(target, otherSub, analysis));
@@ -981,8 +1003,10 @@ TEST_P(RemarkHistoryTest, OwnsQueryAndHistoryStrings) {
{
std::string name = "Candidate", category = "Opt", sub = "Vector";
std::string function = "kernel";
- query =
- query.name(name).category(category).subCategory(sub).function(function);
+ query = remark::RemarkQuery::named(name)
+ .category(category)
+ .subCategory(sub)
+ .function(function);
target = emit(loc, remark::RemarkOpts::name(name)
.category(category)
.subCategory(sub)
@@ -992,11 +1016,32 @@ TEST_P(RemarkHistoryTest, OwnsQueryAndHistoryStrings) {
sub.assign("Changed");
function.assign("Changed");
}
+ flush();
EXPECT_THAT(engine().findRemarks(query), ElementsAre(target));
const auto original = query;
auto changed = original.name("Other");
EXPECT_THAT(engine().findRemarks(changed), IsEmpty());
EXPECT_THAT(engine().findRemarks(original), ElementsAre(target));
+
+ // Saved queries and copied IDs remain usable after the interned pool resets.
+ auto savedIds = engine().findRemarks(query);
+ for (bool disable : {false, true}) {
+ if (disable) {
+ engine().enableHistory(false);
+ engine().enableHistory();
+ } else {
+ engine().clearHistory();
+ }
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+ auto next = emit(loc, remark::RemarkOpts::name("Candidate")
+ .category("Opt")
+ .subCategory("Vector")
+ .function("kernel"));
+ flush();
+ EXPECT_THAT(engine().findRemarks(original), ElementsAre(next));
+ EXPECT_GT(next.getValue(), target.getValue());
+ EXPECT_THAT(savedIds, ElementsAre(target));
+ }
}
TEST_P(RemarkHistoryTest, QueryLinksSnapshotAndSerialization) {
@@ -1007,6 +1052,7 @@ TEST_P(RemarkHistoryTest, QueryLinksSnapshotAndSerialization) {
emit(loc, remark::RemarkOpts::name("First").category("Opt"), true);
auto second =
emit(loc, remark::RemarkOpts::name("Second").category("Opt"), true);
+ flush();
remark::RemarkId linked, later, pendingId, noMatches, copiedWrapper;
{
auto pending = remark::analysis(
@@ -1022,6 +1068,7 @@ TEST_P(RemarkHistoryTest, QueryLinksSnapshotAndSerialization) {
emit(loc, remark::RemarkOpts::name("Later").category("Opt"), true);
}
}
+ flush();
{
const auto wrapper = remark::relatedTo(query);
auto r = remark::passed(
@@ -1044,7 +1091,12 @@ TEST_P(RemarkHistoryTest, QueryLinksSnapshotAndSerialization) {
ElementsAre(std::to_string(first.getValue()),
std::to_string(second.getValue())));
} else if (record.id == copiedWrapper) {
- EXPECT_THAT(record.related, ElementsAre(first, second, later, pendingId));
+ if (GetParam())
+ EXPECT_THAT(record.related,
+ ElementsAre(first, second, pendingId, later));
+ else
+ EXPECT_THAT(record.related,
+ ElementsAre(first, second, later, pendingId));
} else if (record.id == noMatches) {
EXPECT_THAT(record.related, IsEmpty());
EXPECT_THAT(record.serializedLinks, IsEmpty());
@@ -1059,7 +1111,7 @@ TEST_P(RemarkHistoryTest, QueryLinksDoNotEnableHistory) {
Location loc = UnknownLoc::get(&context);
auto query = remark::RemarkQuery{};
emit(loc, remark::RemarkOpts::name("First").category("Opt"));
- remark::RemarkId linked;
+ remark::RemarkId linked, latest;
{
auto r =
remark::passed(loc, remark::RemarkOpts::name("Linked").category("Opt"));
@@ -1067,16 +1119,205 @@ TEST_P(RemarkHistoryTest, QueryLinksDoNotEnableHistory) {
auto wrapper = remark::relatedTo(query);
r << wrapper;
}
+ {
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("LatestDisabled").category("Opt"));
+ latest = r.getId();
+ r << remark::relatedToLatest(query);
+ }
// Both filtered and ownerless remarks accept the lazy wrapper harmlessly.
remark::passed(loc, remark::RemarkOpts::name("Filtered").category("Other"))
<< remark::relatedTo(query);
remark::detail::InFlightRemark{} << remark::relatedTo(query);
+ remark::detail::InFlightRemark{} << remark::relatedToLatest(query);
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+ flush();
engine().enableHistory();
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
- flush();
EXPECT_THAT(records, Contains(AllOf(Field(&Record::id, linked),
Field(&Record::related, IsEmpty()))));
+ expectLinks(latest, {});
+}
+
+TEST_P(RemarkHistoryTest, HistoryEnableAndClearUseEmissionBoundary) {
+ Location loc = UnknownLoc::get(&context);
+ auto query = remark::RemarkQuery{};
+ auto beforeEnable =
+ emit(loc, remark::RemarkOpts::name("BeforeEnable").category("Opt"));
+ engine().enableHistory();
+ flush();
+ if (GetParam())
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(beforeEnable));
+ else
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+
+ auto beforeClear =
+ emit(loc, remark::RemarkOpts::name("BeforeClear").category("Opt"));
+ engine().clearHistory();
+ flush();
+ if (GetParam())
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(beforeClear));
+ else
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+
+ emit(loc, remark::RemarkOpts::name("BeforeDisable").category("Opt"));
+ engine().enableHistory(false);
+ flush();
+ engine().enableHistory();
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+}
+
+TEST_P(RemarkHistoryTest, SameKeyQueriesLinkOnlyEmittedTargets) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
+ auto query = remark::RemarkQuery::named("Candidate").category("Opt");
+ auto first = emit(loc, opts);
+ auto second = emit(loc, opts);
+ if (GetParam())
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+ else
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(first, second));
+
+ remark::RemarkId beforeFlush;
+ {
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("BeforeFlush").category("Opt"));
+ beforeFlush = r.getId();
+ r << remark::relatedTo(query);
+ }
+ flush();
+ if (GetParam()) {
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(second));
+ EXPECT_THAT(records, Not(Contains(Field(&Record::id, first))));
+ expectLinks(beforeFlush, {});
+ } else {
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(first, second));
+ expectLinks(beforeFlush, {first, second});
+ }
+
+ // The same identity can link to an earlier batch without suppressing it.
+ remark::RemarkId sameKey;
+ {
+ auto r = remark::passed(loc, opts);
+ sameKey = r.getId();
+ r << remark::relatedToLatest(query);
+ }
+ flush();
+ expectLinks(sameKey, {second});
+
+ // A pending FINAL replacement must not become a dangling query target.
+ auto replaced = emit(loc, opts);
+ remark::RemarkId consumer;
+ {
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("Consumer").category("Opt"));
+ consumer = r.getId();
+ r << remark::relatedToLatest(query);
+ emit(loc, opts);
+ }
+ flush();
+ expectLinks(consumer, {GetParam() ? sameKey : replaced});
+ if (GetParam())
+ EXPECT_THAT(records, Not(Contains(Field(&Record::id, replaced))));
+ for (const Record &record : records)
+ for (remark::RemarkId related : record.related)
+ EXPECT_THAT(records, Contains(Field(&Record::id, related)));
+}
+
+TEST_P(RemarkHistoryTest, LatestLinkUsesEmissionOrderAndInsertionTime) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto query = remark::RemarkQuery{}.kind(remark::RemarkKind::RemarkAnalysis);
+ remark::RemarkId firstCreated, secondCreated;
+ {
+ auto pending = remark::analysis(
+ loc, remark::RemarkOpts::name("First").category("Opt"));
+ firstCreated = pending.getId();
+ secondCreated =
+ emit(loc, remark::RemarkOpts::name("Second").category("Opt"), true);
+ }
+ flush();
+ ASSERT_LT(firstCreated.getValue(), secondCreated.getValue());
+ auto latest = GetParam() ? secondCreated : firstCreated;
+ remark::RemarkId linked;
+ {
+ const auto wrapper = remark::relatedToLatest(query);
+ auto r =
+ remark::passed(loc, remark::RemarkOpts::name("Linked").category("Opt"));
+ linked = r.getId();
+ r << wrapper;
+ emit(loc, remark::RemarkOpts::name("Later").category("Opt"), true);
+ // Even a later emission before this remark is reported cannot retarget it.
+ flush();
+ }
+ flush();
+ expectLinks(linked, {latest});
+
+ const auto savedWrapper = remark::relatedToLatest(query);
+ engine().clearHistory();
+ remark::RemarkId noMatch;
+ {
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("NoMatch").category("Opt"));
+ noMatch = r.getId();
+ r << savedWrapper;
+ }
+ flush();
+ expectLinks(noMatch, {});
+
+ auto fresh =
+ emit(loc, remark::RemarkOpts::name("Fresh").category("Opt"), true);
+ flush();
+ remark::RemarkId afterReset;
+ {
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("AfterReset").category("Opt"));
+ afterReset = r.getId();
+ r << savedWrapper;
+ }
+ flush();
+ expectLinks(afterReset, {fresh});
+}
+
+TEST_P(RemarkHistoryTest, RepeatedDeliveryRetainsFirstEmissionOrder) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto child =
+ emit(loc, remark::RemarkOpts::name("Child").category("Opt"), true);
+ auto firstParent = emit(
+ loc,
+ remark::RemarkOpts::name("FirstParent").category("Opt").relatedTo(child));
+ auto secondParent = emit(
+ loc,
+ remark::RemarkOpts::name("SecondParent").category("Opt").relatedTo(child),
+ true);
+ flush();
+ auto query = remark::RemarkQuery{};
+ if (GetParam()) {
+ EXPECT_THAT(engine().findRemarks(query),
+ ElementsAre(firstParent, child, secondParent));
+ EXPECT_EQ(
+ llvm::count_if(records, [&](const Record &r) { return r.id == child; }),
+ 2);
+ } else {
+ EXPECT_THAT(engine().findRemarks(query),
+ ElementsAre(child, firstParent, secondParent));
+ }
+
+ // FINAL delivered Child again after SecondParent; it is still the older
+ // match.
+ auto analyses = query.kind(remark::RemarkKind::RemarkAnalysis);
+ EXPECT_THAT(engine().findRemarks(analyses), ElementsAre(child, secondParent));
+ remark::RemarkId linked;
+ {
+ auto r =
+ remark::passed(loc, remark::RemarkOpts::name("Latest").category("Opt"));
+ linked = r.getId();
+ r << remark::relatedToLatest(analyses);
+ }
+ flush();
+ expectLinks(linked, {secondParent});
}
TEST_P(RemarkHistoryTest, StreamerCanQueryCurrentReport) {
@@ -1093,6 +1334,28 @@ INSTANTIATE_TEST_SUITE_P(EmittingPolicies, RemarkHistoryTest, Bool(),
return info.param ? "Final" : "All";
});
+TEST(Remark, RawMetadataAccessorsPreserveEmptyFields) {
+ MLIRContext context;
+ Location loc = UnknownLoc::get(&context);
+ remark::detail::OptRemarkPass empty(loc, remark::RemarkOpts{});
+ EXPECT_TRUE(empty.getRawRemarkName().empty());
+ EXPECT_TRUE(empty.getRawFunction().empty());
+ EXPECT_TRUE(empty.getSubCategoryName().empty());
+ EXPECT_EQ(empty.getRemarkName(), "<unknown remark name>");
+ EXPECT_EQ(empty.getFunction(), "<unknown function>");
+
+ remark::detail::OptRemarkPass populated(loc,
+ remark::RemarkOpts::name("Candidate")
+ .category("Opt")
+ .subCategory("Vector")
+ .function("kernel"));
+ EXPECT_EQ(populated.getRawRemarkName(), "Candidate");
+ EXPECT_EQ(populated.getRawFunction(), "kernel");
+ EXPECT_EQ(populated.getCategoryName(), "Opt");
+ EXPECT_EQ(populated.getSubCategoryName(), "Vector");
+ EXPECT_EQ(populated.getCombinedCategoryName(), "Opt:Vector");
+}
+
TEST(Remark, TestNestedLocation) {
MLIRContext context;
Location fileLoc = FileLineColLoc::get(&context, "test.mlir", 42, 7);
>From 35f74fd27d4b034b3abb99a3c8d608fbaff3efbf Mon Sep 17 00:00:00 2001
From: Guray Ozen <gozen at nvidia.com>
Date: Tue, 3 Nov 2026 11:41:49 +0100
Subject: [PATCH 3/4] [mlir] Query pending Final remarks and prune discarded
links
---
mlir/docs/Remarks.md | 104 ++++++----
mlir/include/mlir/IR/Remarks.h | 90 ++++-----
mlir/lib/IR/Remarks.cpp | 210 +++++++++++++------
mlir/unittests/IR/RemarkTest.cpp | 337 ++++++++++++++++++++++---------
4 files changed, 496 insertions(+), 245 deletions(-)
diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md
index 0ff919588f08bc..e18b88124bfadc 100644
--- a/mlir/docs/Remarks.md
+++ b/mlir/docs/Remarks.md
@@ -202,24 +202,35 @@ Emits **all** remarks unconditionally.
### RemarkEmittingPolicyFinal
-Stores remarks until `finalize()` is called and emits only the **final** remark
-for each location. This is useful in multi-pass compilers where an early pass
-may report a failure, but a later pass succeeds. `finalize()` drains the stored
-remarks and emits them in the order in which they were created. Calling it
-again emits only remarks reported since.
+Stores remarks until `finalize()` is called and keeps only the **last reported**
+remark for each identity: location, remark name, combined category (including
+its subcategory), and remark kind. Function names are not part of this identity.
+Remarks of different kinds do not replace one another.
-**Example:** Only the successful remark is emitted:
+**Example:** Only the second missed remark is emitted:
```c++
auto opts = remark::RemarkOpts::name("Unroller").category("LoopUnroll");
-// First pass: reports failure
-remark::failed(loc, opts) << "Loop could not be unrolled";
+// First pass: the trip count is unknown.
+remark::missed(loc, opts) << "Cannot unroll: trip count is unknown";
-// Later pass: reports success (this is the one emitted)
-remark::passed(loc, opts) << "Loop unrolled successfully";
+// Later pass: the remaining obstacle has changed.
+remark::missed(loc, opts) << "Cannot unroll: code size limit exceeded";
```
+`finalize()` drains the stored remarks, visits top-level remarks in creation
+order, and groups related surviving remarks after the remarks that reference
+them. It follows links transitively and emits each survivor once, including
+when links form a cycle. Calling `finalize()` again emits only remarks reported
+since the previous call.
+
+Before emission, the policy removes links to discarded or unknown IDs, including
+their serialized `RelatedTo` arguments. It does not redirect a link to a
+replacement remark or discard a remark because its target was discarded. Links
+to targets emitted by an earlier `finalize()` remain valid without emitting
+those targets again.
+
You can also implement custom policies by inheriting from the policy interface.
***
@@ -253,18 +264,23 @@ if (engine->isRemarkEnabled(remark::RemarkKind::RemarkPassed, "Vectorizer"))
## Searching and Linking Remarks
-History is opt-in and records each emitted event ID once, in first-emission
-order, before the streamer callback. It retains IDs and identifying metadata,
-with interned strings, but not messages or arguments. `All` delivers reports
-immediately; `Final` makes them searchable only when finalized and excludes
-discarded events. Previously emitted targets remain searchable across batches.
-Enable history before emission: pending `Final` remarks can still be recorded
-when delivered, but earlier emissions and engine-filtered remarks are not recovered.
+History is opt-in. When enabled, queries search both previously emitted remarks
+and the current pending remarks held by `Final`. A pending remark is searchable
+as soon as it is reported; if another report replaces it, subsequent queries
+see the replacement. The engine stores emitted IDs and identifying metadata,
+with interned strings, but not messages or arguments. `Final` owns its pending
+full remarks independently of this metadata cache.
+
+Each matching ID is returned once, in creation-ID order. Previously emitted
+targets remain searchable across batches until history is cleared or disabled.
+Enabling history also exposes existing pending `Final` remarks, but cannot
+recover earlier emissions or engine-filtered remarks.
`RemarkQuery` matches name, base category, subcategory, function, kind, and
complete location, including wrappers. Specified fields use exact AND matching;
unspecified fields match anything, and an empty string matches empty metadata.
-For example, with an already configured `All` engine and enabled categories:
+For example, with an already configured `All` or `Final` engine and enabled
+categories:
```c++
auto *engine = context.getRemarkEngine();
@@ -280,35 +296,49 @@ auto query = remark::RemarkQuery::named("VectorSizeInference")
.function("kernel")
.kind(remark::RemarkKind::RemarkAnalysis)
.at(loc);
-// Matching IDs are returned in first-emission order.
+// Matching IDs are returned in creation-ID order.
SmallVector<remark::RemarkId> matches = engine->findRemarks(query);
remark::missed(loc, remark::RemarkOpts::name("Vectorization")
.category("Vectorizer")
.function("kernel"))
- << remark::relatedToLatest(query);
+ << remark::relatedTo(query);
```
-`relatedTo(query)` links all matches; `relatedToLatest(query)` links at most one,
-the most recent matching first-emission entry. Both resolve at `<<`; no match
-or disabled history adds no links, and later emissions do not change them.
-Known IDs can use `RemarkOpts::relatedTo(id)`. Serialized links repeat the
-`RelatedTo` key, so consumers must preserve duplicates. Matching does not establish
-causality, operation identity, or continued validity after IR changes.
+`relatedTo(query)` links all matches and resolves at `<<`; no match or disabled
+history adds no links. It does not rerun the query when the remark is emitted.
+Known IDs can use `RemarkOpts::relatedTo(id)`. Under `Final`, links to targets
+that are subsequently replaced are removed during finalization. A query result
+therefore describes the current candidates, not a guarantee of eventual
+emission.
+
+For example, suppose `r1` and `r2` have the same Final identity, `r3` has a
+different identity, and all three are reported before finalization:
+
+| Report and query order | Final output |
+|------------------------|--------------|
+| Report `r1`; query and attach `r3 → r1`; report replacement `r2` | `r2` and `r3`, with the link removed |
+| Report `r1`; report replacement `r2`; query and attach `r3 → r2` | `r2` and `r3`, with the link preserved |
+
+Serialized links repeat the `RelatedTo` key, so consumers must preserve
+duplicates. Matching does not establish causality, operation identity, or
+continued validity after IR changes.
Queries own their criteria strings; queries with locations must not outlive the
locations' `MLIRContext`.
History access is synchronized, including queries from streamer callbacks that
-see the current event. This does not synchronize policies or streamers. With a
-parallel pass manager, cross-function query membership and ordering depend on
-timing; scope queries to a function and sequence producers or use explicit
-barriers. First-emission order can differ from ID creation order.
-
-Memory is retained until `clearHistory()`, `enableHistory(false)`, or engine
-destruction. Clearing keeps recording; disabling stops it. Neither resets IDs,
-changes existing links, nor cancels pending `Final` remarks. Repeated enabling
-preserves entries. Latest-match search scans backward and stops at its first
-match, but remains O(N) in the worst case. Repeated broad all-match queries can
-produce quadratic numbers of links.
+see the current event. This does not make custom policies or streamers
+thread-safe. With a parallel pass manager, cross-function query membership and
+ID creation order depend on timing; scope queries to a function and sequence
+producers or use explicit barriers.
+
+Emitted metadata is retained until `clearHistory()`, `enableHistory(false)`, or
+engine destruction. Clearing keeps recording, and pending `Final` remarks remain
+searchable. Disabling stops recording and makes queries return no matches.
+Neither operation resets IDs, changes existing links, or cancels pending
+`Final` remarks. Repeated enabling preserves entries. `Final` independently
+remembers which IDs it emitted so that existing links to earlier batches remain
+valid even after history is cleared or disabled. Repeated broad all-match
+queries can produce quadratic numbers of links.
***
diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 49c5e1ff5abd70..2c5bfa3befa669 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -142,7 +142,7 @@ struct RemarkOpts {
inline RemarkOpts relatedTo(const detail::InFlightRemark &r) const;
};
-/// Exact-match criteria for searching previously emitted remarks. Unspecified
+/// Exact-match criteria for searching emitted or pending remarks. Unspecified
/// fields match any value; a specified empty string matches an empty field.
/// Strings are owned by the query. Locations belong to their MLIRContext and
/// must not outlive it. A location match does not imply operation identity.
@@ -341,6 +341,9 @@ class Remark {
}
}
+ /// Remove links rejected by the predicate, including their serialized args.
+ void retainRelatedRemarks(llvm::function_ref<bool(RemarkId)> shouldKeep);
+
/// Check if this remark has any related remarks.
bool hasRelatedRemarks() const { return !relatedRemarks.empty(); }
@@ -467,7 +470,6 @@ struct LazyTextBuild {
/// Resolves a query when inserted into an in-flight remark.
struct RelatedRemarkQuery {
RemarkQuery query;
- bool latestOnly = false;
};
/// InFlightRemark is a RAII class that holds a reference to a Remark
@@ -561,13 +563,19 @@ class RemarkEmittingPolicyBase {
virtual void reportRemark(const Remark &remark) = 0;
virtual void finalize() = 0;
- /// Legacy policy-specific lookup hook. Engine history queries use the
- /// records actually emitted by the policy, independently of this hook.
+ /// Legacy policy-specific lookup hook. Engine history queries use emitted
+ /// metadata and forEachPendingRemark, independently of this hook.
virtual SmallVector<RemarkId>
findRemarks(const RemarkOpts &opts,
std::optional<RemarkKind> kind = std::nullopt) const {
return {};
}
+
+ /// Visit remarks retained for future emission. The callback must not reenter
+ /// the policy or retain references to the remarks. Policies may hold a lock
+ /// while invoking it. The default policy has no pending remarks.
+ virtual void forEachPendingRemark(
+ llvm::function_ref<void(const Remark &)> callback) const {}
};
//===----------------------------------------------------------------------===//
@@ -576,8 +584,6 @@ class RemarkEmittingPolicyBase {
class RemarkEngine {
private:
- friend class InFlightRemark;
-
/// Regex that filters missed optimization remarks: only matching one are
/// reported.
std::optional<llvm::Regex> missFilter;
@@ -602,12 +608,6 @@ class RemarkEngine {
std::unique_ptr<RemarkHistory> history;
std::atomic<bool> historyEnabled{false};
- /// Visit matches while holding the history lock. Only internal callbacks that
- /// do not reenter the engine may be used here.
- void forEachMatch(const RemarkQuery &query,
- llvm::function_ref<void(RemarkId)> callback,
- bool latestOnly = false) const;
-
/// Emit a remark using the given maker function, which should return
/// a Remark instance. The remark will be emitted using the main
/// remark streamer.
@@ -690,26 +690,28 @@ class RemarkEngine {
}
//===--------------------------------------------------------------------===//
- // Remark Linking - query previously emitted remarks
+ // Remark Linking - query emitted and pending remarks
//===--------------------------------------------------------------------===//
- /// Enable recording metadata for subsequent policy emissions. Repeated
- /// enabling preserves history. Disabling discards history. No past events are
- /// recovered. History is retained until cleared, disabled, or the engine is
- /// destroyed; callers reusing an engine across compilations should clear it
- /// between runs.
+ /// Enable history queries and recording metadata for subsequent emissions.
+ /// Pending policy remarks become searchable immediately, including those
+ /// reported before enabling. Earlier emissions are not recovered. Repeated
+ /// enabling preserves history; disabling discards emitted metadata and
+ /// suppresses queries without cancelling pending policy remarks.
void enableHistory(bool enable = true);
- /// Discard recorded metadata without changing whether history is enabled.
- /// Existing remark IDs and links are unaffected.
+ /// Discard emitted metadata without changing whether history is enabled.
+ /// Pending policy remarks remain searchable. Existing IDs and links are
+ /// unaffected.
void clearHistory();
- /// Find distinct emitted remarks matching every specified criterion, in
- /// first-emission order. Pending, engine-filtered and policy-discarded
- /// remarks are absent. Returns empty when history is disabled. Policy
- /// finalization may add entries but does not clear history. A callback can
- /// query the event currently being delivered. History access is synchronized;
- /// this does not make emitting policies or streamers thread-safe.
+ /// Find distinct emitted and pending remarks matching every specified
+ /// criterion, in ID creation order. Final exposes its current survivors;
+ /// replacement removes the old pending event from subsequent queries.
+ /// Returns empty when history is disabled. Finalization may add emitted
+ /// metadata but does not clear history. A streamer callback can query the
+ /// current event and the remaining pending remarks. History access is
+ /// synchronized; this does not make custom policies or streamers thread-safe.
///
/// Cross-function queries during parallel pass execution have
/// timing-dependent results and ordering. Scope queries with function() and
@@ -766,20 +768,14 @@ mlir::remark::RemarkOpts::relatedTo(const detail::InFlightRemark &r) const {
namespace mlir::remark {
/// Link to all remarks matching the query when this helper is inserted into an
-/// in-flight remark. History must have been enabled before the policy emitted
-/// the remarks. No matches (including disabled history) adds no links. Matching
+/// in-flight remark. History must be enabled. No matches (including disabled
+/// history) adds no links. Final removes links to targets discarded before
+/// emission; links are never redirected to their replacements. Matching
/// indicates a relationship, not causality or operation identity.
inline detail::RelatedRemarkQuery relatedTo(RemarkQuery query) {
return {std::move(query)};
}
-/// Link only the latest match in first-emission order. The reverse search stops
-/// at the first match and adds at most one RelatedTo argument. Repeated
-/// delivery of an existing event does not refresh its position in history.
-inline detail::RelatedRemarkQuery relatedToLatest(RemarkQuery query) {
- return {std::move(query), true};
-}
-
//===----------------------------------------------------------------------===//
// Remark Emitting Policies
//===----------------------------------------------------------------------===//
@@ -798,24 +794,26 @@ class RemarkEmittingPolicyAll : public detail::RemarkEmittingPolicyBase {
/// Policy that emits only the last remark reported for each identity, see
/// DenseMapInfo<Remark>. Remarks are stored until finalize(), which emits them
-/// in creation order, so the output does not depend on hash order.
+/// in creation order, grouping related remarks after their parents. Links to
+/// discarded targets are removed; they do not invalidate their source remark.
class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
private:
- /// Remarks reported since the last finalize().
- llvm::DenseSet<detail::Remark> postponedRemarks;
+ struct PendingRemarks;
+ std::unique_ptr<PendingRemarks> pending;
public:
RemarkEmittingPolicyFinal();
+ ~RemarkEmittingPolicyFinal() override;
- void reportRemark(const detail::Remark &remark) override {
- postponedRemarks.erase(remark);
- postponedRemarks.insert(remark);
- }
+ void reportRemark(const detail::Remark &remark) override;
+
+ void forEachPendingRemark(
+ llvm::function_ref<void(const detail::Remark &)> callback) const override;
- /// Emits and drains all stored remarks. Related remarks are printed right
- /// after the remark that references them; a link only resolves when both
- /// remarks are in the same call. A later call emits only remarks reported
- /// since this one.
+ /// Emit each survivor once, following related remarks transitively. Links
+ /// may reference current survivors or previously emitted IDs. Other links
+ /// are removed. Previously emitted targets are not repeated. Cycles do not
+ /// suppress output. A later call emits only newly reported remarks.
void finalize() override;
};
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index d912d35020e31f..8164000fd83869 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -12,10 +12,12 @@
#include "mlir/IR/Diagnostics.h"
#include "mlir/IR/Value.h"
+#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/StringSaver.h"
+#include <algorithm>
#include <mutex>
#include <vector>
@@ -43,6 +45,24 @@ Remark::Arg::Arg(llvm::StringRef k, Attribute a) : key(k), attr(a) {
void Remark::insert(llvm::StringRef s) { args.emplace_back(s); }
void Remark::insert(Arg a) { args.push_back(std::move(a)); }
+void Remark::retainRelatedRemarks(
+ llvm::function_ref<bool(RemarkId)> shouldKeep) {
+ llvm::SmallDenseSet<uint64_t> removed;
+ llvm::erase_if(relatedRemarks, [&](RemarkId id) {
+ if (shouldKeep(id))
+ return false;
+ removed.insert(id.getValue());
+ return true;
+ });
+ if (removed.empty())
+ return;
+ llvm::erase_if(args, [&](const Arg &arg) {
+ uint64_t id;
+ return arg.key == "RelatedTo" && !StringRef(arg.val).getAsInteger(10, id) &&
+ removed.contains(id);
+ });
+}
+
// Simple helper to print key=val list (sorted).
static void printArgs(llvm::raw_ostream &os, llvm::ArrayRef<Remark::Arg> args) {
if (args.empty())
@@ -170,9 +190,8 @@ llvm::remarks::Remark Remark::generateRemark() const {
InFlightRemark &InFlightRemark::operator<<(const RelatedRemarkQuery &related) {
if (remark && owner)
- owner->forEachMatch(
- related.query, [this](RemarkId id) { remark->addRelatedRemark(id); },
- related.latestOnly);
+ for (RemarkId id : owner->findRemarks(related.query))
+ remark->addRelatedRemark(id);
return *this;
}
@@ -193,14 +212,20 @@ struct RemarkEngine::RemarkHistory {
Location location;
StringRef name, category, subCategory, function;
+ static Entry from(const Remark &remark) {
+ return {remark.getId(), remark.getRemarkKind(),
+ remark.getLocation(), remark.getRawRemarkName(),
+ remark.getCategoryName(), remark.getSubCategoryName(),
+ remark.getRawFunction()};
+ }
+
static Entry from(const Remark &remark, llvm::UniqueStringSaver &strings) {
- return {remark.getId(),
- remark.getRemarkKind(),
- remark.getLocation(),
- strings.save(remark.getRawRemarkName()),
- strings.save(remark.getCategoryName()),
- strings.save(remark.getSubCategoryName()),
- strings.save(remark.getRawFunction())};
+ Entry entry = from(remark);
+ entry.name = strings.save(entry.name);
+ entry.category = strings.save(entry.category);
+ entry.subCategory = strings.save(entry.subCategory);
+ entry.function = strings.save(entry.function);
+ return entry;
}
};
@@ -218,7 +243,7 @@ struct RemarkEngine::RemarkHistory {
};
void RemarkEngine::enableHistory(bool enable) {
- std::scoped_lock lock(history->mutex);
+ std::scoped_lock<std::mutex> lock(history->mutex);
if (enable) {
if (!history->storage)
history->storage = std::make_unique<RemarkHistory::Storage>();
@@ -231,19 +256,16 @@ void RemarkEngine::enableHistory(bool enable) {
void RemarkEngine::clearHistory() {
if (!historyEnabled.load(std::memory_order_acquire))
return;
- std::scoped_lock lock(history->mutex);
+ std::scoped_lock<std::mutex> lock(history->mutex);
if (history->storage)
history->storage = std::make_unique<RemarkHistory::Storage>();
}
-void RemarkEngine::forEachMatch(const RemarkQuery &query,
- llvm::function_ref<void(RemarkId)> callback,
- bool latestOnly) const {
+SmallVector<RemarkId>
+RemarkEngine::findRemarks(const RemarkQuery &query) const {
+ SmallVector<RemarkId> result;
if (!historyEnabled.load(std::memory_order_acquire))
- return;
- std::scoped_lock lock(history->mutex);
- if (!history->storage)
- return;
+ return result;
auto matches = [](const auto &filter, const auto &value) {
return !filter || *filter == value;
@@ -256,25 +278,27 @@ void RemarkEngine::forEachMatch(const RemarkQuery &query,
matches(query.remarkKind, entry.kind) &&
matches(query.location, entry.location);
};
- const auto &entries = history->storage->entries;
- if (latestOnly) {
- for (const RemarkHistory::Entry &entry : llvm::reverse(entries)) {
- if (!matchesQuery(entry))
- continue;
- callback(entry.id);
- break;
- }
- return;
- }
- for (const RemarkHistory::Entry &entry : entries)
- if (matchesQuery(entry))
- callback(entry.id);
-}
+ // Read pending remarks before emitted history: a remark delivered between
+ // these snapshots can appear in both, but cannot disappear between them.
+ // Do not hold the history mutex while calling a policy.
+ if (remarkEmittingPolicy)
+ remarkEmittingPolicy->forEachPendingRemark([&](const Remark &remark) {
+ if (remark.getId() && matchesQuery(RemarkHistory::Entry::from(remark)))
+ result.push_back(remark.getId());
+ });
-SmallVector<RemarkId>
-RemarkEngine::findRemarks(const RemarkQuery &query) const {
- SmallVector<RemarkId> result;
- forEachMatch(query, [&](RemarkId id) { result.push_back(id); });
+ {
+ std::scoped_lock<std::mutex> lock(history->mutex);
+ if (!history->storage)
+ return {};
+ for (const RemarkHistory::Entry &entry : history->storage->entries)
+ if (matchesQuery(entry))
+ result.push_back(entry.id);
+ }
+ llvm::sort(result, [](RemarkId lhs, RemarkId rhs) {
+ return lhs.getValue() < rhs.getValue();
+ });
+ result.erase(std::unique(result.begin(), result.end()), result.end());
return result;
}
@@ -375,10 +399,10 @@ InFlightRemark RemarkEngine::emitOptimizationRemarkAnalysis(Location loc,
void RemarkEngine::reportImpl(const Remark &remark) {
if (historyEnabled.load(std::memory_order_acquire)) {
- std::scoped_lock lock(history->mutex);
+ std::scoped_lock<std::mutex> lock(history->mutex);
if (history->storage && remark.getId()) {
auto &storage = *history->storage;
- // FINAL can emit a shared related remark more than once. Keep one history
+ // Custom policies can deliver an event more than once. Retain one history
// entry per event so a query does not produce duplicate links.
if (storage.emittedIds.insert(remark.getId().getValue()).second)
storage.entries.push_back(
@@ -499,21 +523,62 @@ llvm::LogicalResult mlir::remark::enableOptimizationRemarks(
namespace mlir::remark {
RemarkEmittingPolicyAll::RemarkEmittingPolicyAll() = default;
-RemarkEmittingPolicyFinal::RemarkEmittingPolicyFinal() = default;
+
+struct RemarkEmittingPolicyFinal::PendingRemarks {
+ std::mutex mutex;
+ llvm::DenseSet<detail::Remark> remarks;
+ // Keep the batch being delivered queryable during streamer callbacks.
+ llvm::DenseSet<const detail::Remark *> draining;
+ // IDs committed to output, including active drains. This survives history
+ // clearing and disabling so links to earlier batches remain valid.
+ llvm::DenseSet<uint64_t> emittedIds;
+};
+
+RemarkEmittingPolicyFinal::RemarkEmittingPolicyFinal()
+ : pending(std::make_unique<PendingRemarks>()) {}
+
+RemarkEmittingPolicyFinal::~RemarkEmittingPolicyFinal() = default;
+
+void RemarkEmittingPolicyFinal::reportRemark(const detail::Remark &remark) {
+ std::scoped_lock<std::mutex> lock(pending->mutex);
+ pending->remarks.erase(remark);
+ pending->remarks.insert(remark);
+}
+
+void RemarkEmittingPolicyFinal::forEachPendingRemark(
+ llvm::function_ref<void(const detail::Remark &)> callback) const {
+ std::scoped_lock<std::mutex> lock(pending->mutex);
+ for (const auto &remark : pending->remarks)
+ callback(remark);
+ for (const auto *remark : pending->draining)
+ callback(*remark);
+}
void RemarkEmittingPolicyFinal::finalize() {
assert(reportImpl && "reportImpl is not set");
- // Take the pending remarks so that a second finalize(), e.g. from the engine
- // destructor after an explicit call, does not emit them again. IDs are
- // assigned in creation order; sorting by them keeps the output independent
- // of the set's hash layout.
- std::vector<detail::Remark> remarks(postponedRemarks.begin(),
- postponedRemarks.end());
- postponedRemarks.clear();
- llvm::sort(remarks, [](const auto &lhs, const auto &rhs) {
- return lhs.getId().getValue() < rhs.getId().getValue();
- });
+ std::vector<detail::Remark> remarks;
+ {
+ std::scoped_lock<std::mutex> lock(pending->mutex);
+ // Drain the batch before invoking callbacks, which may report more remarks.
+ remarks.assign(pending->remarks.begin(), pending->remarks.end());
+ pending->remarks.clear();
+ llvm::sort(remarks, [](const auto &lhs, const auto &rhs) {
+ return lhs.getId().getValue() < rhs.getId().getValue();
+ });
+
+ for (const auto &remark : remarks)
+ if (remark.getId())
+ pending->emittedIds.insert(remark.getId().getValue());
+ for (auto &remark : remarks) {
+ // A reference does not keep a superseded target alive or invalidate its
+ // source. Remove dangling links without redirecting them to replacements.
+ remark.retainRelatedRemarks([&](RemarkId id) {
+ return pending->emittedIds.contains(id.getValue());
+ });
+ pending->draining.insert(&remark);
+ }
+ }
// Build ID -> Remark* lookup for resolving related remark references.
llvm::DenseMap<uint64_t, const detail::Remark *> idMap;
@@ -526,22 +591,41 @@ void RemarkEmittingPolicyFinal::finalize() {
childIds.insert(relId.getValue());
}
- // Emit remarks with related remarks grouped after their parents.
- // Parent remarks are emitted first, followed by their related (child)
- // remarks. Child-only remarks are skipped at the top level to avoid
- // duplication.
+ // Emit parents before their related remarks, following chains transitively.
+ // Visit by address so ID-less remarks reported directly to the policy are
+ // also emitted. A shared target is emitted only once.
+ llvm::DenseSet<const detail::Remark *> emitted;
+ SmallVector<const detail::Remark *> worklist;
+ auto emit = [&](const detail::Remark &root) {
+ worklist.push_back(&root);
+ while (!worklist.empty()) {
+ const auto *remark = worklist.pop_back_val();
+ if (!emitted.insert(remark).second)
+ continue;
+ // Never hold the policy mutex across callbacks: they may query pending
+ // remarks or report new ones. Delivery records history before this
+ // remark is removed from the pending snapshot.
+ reportImpl(*remark);
+ {
+ std::scoped_lock<std::mutex> lock(pending->mutex);
+ pending->draining.erase(remark);
+ }
+ for (RemarkId id : llvm::reverse(remark->getRelatedRemarkIds()))
+ if (const auto *related = idMap.lookup(id.getValue()))
+ worklist.push_back(related);
+ }
+ };
+
for (const auto &remark : remarks) {
if (remark.getId() && childIds.contains(remark.getId().getValue()))
- continue; // will be printed grouped under its parent
-
- reportImpl(remark);
-
- // Emit related remarks immediately after the parent.
- for (auto relId : remark.getRelatedRemarkIds()) {
- if (const auto *related = idMap.lookup(relId.getValue()))
- reportImpl(*related);
- }
+ continue;
+ emit(remark);
}
+ // Cycles (including self-links) have no root. Emit any remaining components
+ // in creation order instead of silently dropping every member of a cycle.
+ for (const auto &remark : remarks)
+ if (!emitted.contains(&remark))
+ emit(remark);
}
} // namespace mlir::remark
diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp
index 9b1c4240165411..d7a55dcd6316ad 100644
--- a/mlir/unittests/IR/RemarkTest.cpp
+++ b/mlir/unittests/IR/RemarkTest.cpp
@@ -495,9 +495,8 @@ TEST(Remark, TestRemarkFinalDrains) {
"First: first again"));
}
-// A RelatedTo link only resolves between remarks drained by the same
-// finalize() call. A parent reported after its child was drained is emitted
-// without the child being repeated.
+// A RelatedTo link to an earlier batch stays valid. The target is not emitted
+// again when the parent is drained.
TEST(Remark, TestRemarkFinalLinkAcrossDrains) {
std::vector<std::string> emitted;
{
@@ -823,6 +822,7 @@ class RemarkHistoryTest : public TestWithParam<bool> {
void SetUp() override {
remark::RemarkCategories categories;
categories.passed = "(Opt.*)?";
+ categories.missed = "(Opt.*)?";
categories.analysis = "(Opt.*)?";
std::unique_ptr<remark::detail::RemarkEmittingPolicyBase> policy;
if (GetParam())
@@ -856,6 +856,11 @@ class RemarkHistoryTest : public TestWithParam<bool> {
return r.getId();
}
+ remark::RemarkId emitMissed(Location loc, remark::RemarkOpts opts) {
+ auto r = remark::missed(loc, opts);
+ return r.getId();
+ }
+
void flush() { engine().getRemarkEmittingPolicy()->finalize(); }
void expectLinks(remark::RemarkId id, ArrayRef<remark::RemarkId> expected) {
@@ -876,7 +881,7 @@ class RemarkHistoryTest : public TestWithParam<bool> {
MLIRContext context;
};
-TEST_P(RemarkHistoryTest, LifecycleAndEmissionOrder) {
+TEST_P(RemarkHistoryTest, LifecycleAndCreationOrder) {
Location loc = UnknownLoc::get(&context);
auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
auto any = remark::RemarkQuery{};
@@ -892,21 +897,16 @@ TEST_P(RemarkHistoryTest, LifecycleAndEmissionOrder) {
EXPECT_THAT(engine().findRemarks(any), IsEmpty());
secondCreated =
emit(loc, remark::RemarkOpts::name("Second").category("Opt"));
- if (GetParam())
- EXPECT_THAT(engine().findRemarks(any), IsEmpty());
- else
- EXPECT_THAT(engine().findRemarks(any), ElementsAre(secondCreated));
+ EXPECT_THAT(engine().findRemarks(any), ElementsAre(secondCreated));
}
EXPECT_LT(firstCreated.getValue(), secondCreated.getValue());
- // ALL emits immediately; FINAL emits these independent records in ID order.
+ // Query ordering follows creation IDs, even when reports arrive out of order.
+ EXPECT_THAT(engine().findRemarks(any),
+ ElementsAre(firstCreated, secondCreated));
flush();
engine().enableHistory();
- if (GetParam())
- EXPECT_THAT(engine().findRemarks(any),
- ElementsAre(firstCreated, secondCreated));
- else
- EXPECT_THAT(engine().findRemarks(any),
- ElementsAre(secondCreated, firstCreated));
+ EXPECT_THAT(engine().findRemarks(any),
+ ElementsAre(firstCreated, secondCreated));
engine().clearHistory();
EXPECT_THAT(engine().findRemarks(any), IsEmpty());
auto afterClear = emit(loc, opts);
@@ -1091,12 +1091,7 @@ TEST_P(RemarkHistoryTest, QueryLinksSnapshotAndSerialization) {
ElementsAre(std::to_string(first.getValue()),
std::to_string(second.getValue())));
} else if (record.id == copiedWrapper) {
- if (GetParam())
- EXPECT_THAT(record.related,
- ElementsAre(first, second, pendingId, later));
- else
- EXPECT_THAT(record.related,
- ElementsAre(first, second, later, pendingId));
+ EXPECT_THAT(record.related, ElementsAre(first, second, pendingId, later));
} else if (record.id == noMatches) {
EXPECT_THAT(record.related, IsEmpty());
EXPECT_THAT(record.serializedLinks, IsEmpty());
@@ -1111,7 +1106,7 @@ TEST_P(RemarkHistoryTest, QueryLinksDoNotEnableHistory) {
Location loc = UnknownLoc::get(&context);
auto query = remark::RemarkQuery{};
emit(loc, remark::RemarkOpts::name("First").category("Opt"));
- remark::RemarkId linked, latest;
+ remark::RemarkId linked;
{
auto r =
remark::passed(loc, remark::RemarkOpts::name("Linked").category("Opt"));
@@ -1119,55 +1114,56 @@ TEST_P(RemarkHistoryTest, QueryLinksDoNotEnableHistory) {
auto wrapper = remark::relatedTo(query);
r << wrapper;
}
- {
- auto r = remark::passed(
- loc, remark::RemarkOpts::name("LatestDisabled").category("Opt"));
- latest = r.getId();
- r << remark::relatedToLatest(query);
- }
// Both filtered and ownerless remarks accept the lazy wrapper harmlessly.
remark::passed(loc, remark::RemarkOpts::name("Filtered").category("Other"))
<< remark::relatedTo(query);
remark::detail::InFlightRemark{} << remark::relatedTo(query);
- remark::detail::InFlightRemark{} << remark::relatedToLatest(query);
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
flush();
engine().enableHistory();
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
EXPECT_THAT(records, Contains(AllOf(Field(&Record::id, linked),
Field(&Record::related, IsEmpty()))));
- expectLinks(latest, {});
}
-TEST_P(RemarkHistoryTest, HistoryEnableAndClearUseEmissionBoundary) {
+TEST_P(RemarkHistoryTest, HistoryControlsPreserveFinalPendingRemarks) {
Location loc = UnknownLoc::get(&context);
auto query = remark::RemarkQuery{};
auto beforeEnable =
emit(loc, remark::RemarkOpts::name("BeforeEnable").category("Opt"));
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
engine().enableHistory();
- flush();
if (GetParam())
EXPECT_THAT(engine().findRemarks(query), ElementsAre(beforeEnable));
else
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+ flush();
auto beforeClear =
emit(loc, remark::RemarkOpts::name("BeforeClear").category("Opt"));
engine().clearHistory();
- flush();
if (GetParam())
EXPECT_THAT(engine().findRemarks(query), ElementsAre(beforeClear));
else
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+ flush();
- emit(loc, remark::RemarkOpts::name("BeforeDisable").category("Opt"));
+ auto beforeDisable =
+ emit(loc, remark::RemarkOpts::name("BeforeDisable").category("Opt"));
+ engine().enableHistory(false);
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+ engine().enableHistory();
+ if (GetParam())
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(beforeDisable));
+ else
+ EXPECT_THAT(engine().findRemarks(query), IsEmpty());
engine().enableHistory(false);
flush();
engine().enableHistory();
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
}
-TEST_P(RemarkHistoryTest, SameKeyQueriesLinkOnlyEmittedTargets) {
+TEST_P(RemarkHistoryTest, SameKeyQueriesLinkPendingAndEmittedTargets) {
engine().enableHistory();
Location loc = UnknownLoc::get(&context);
auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
@@ -1175,7 +1171,7 @@ TEST_P(RemarkHistoryTest, SameKeyQueriesLinkOnlyEmittedTargets) {
auto first = emit(loc, opts);
auto second = emit(loc, opts);
if (GetParam())
- EXPECT_THAT(engine().findRemarks(query), IsEmpty());
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(second));
else
EXPECT_THAT(engine().findRemarks(query), ElementsAre(first, second));
@@ -1190,7 +1186,7 @@ TEST_P(RemarkHistoryTest, SameKeyQueriesLinkOnlyEmittedTargets) {
if (GetParam()) {
EXPECT_THAT(engine().findRemarks(query), ElementsAre(second));
EXPECT_THAT(records, Not(Contains(Field(&Record::id, first))));
- expectLinks(beforeFlush, {});
+ expectLinks(beforeFlush, {second});
} else {
EXPECT_THAT(engine().findRemarks(query), ElementsAre(first, second));
expectLinks(beforeFlush, {first, second});
@@ -1201,10 +1197,13 @@ TEST_P(RemarkHistoryTest, SameKeyQueriesLinkOnlyEmittedTargets) {
{
auto r = remark::passed(loc, opts);
sameKey = r.getId();
- r << remark::relatedToLatest(query);
+ r << remark::relatedTo(query);
}
flush();
- expectLinks(sameKey, {second});
+ if (GetParam())
+ expectLinks(sameKey, {second});
+ else
+ expectLinks(sameKey, {first, second});
// A pending FINAL replacement must not become a dangling query target.
auto replaced = emit(loc, opts);
@@ -1213,74 +1212,211 @@ TEST_P(RemarkHistoryTest, SameKeyQueriesLinkOnlyEmittedTargets) {
auto r = remark::passed(
loc, remark::RemarkOpts::name("Consumer").category("Opt"));
consumer = r.getId();
- r << remark::relatedToLatest(query);
+ r << remark::relatedTo(query);
emit(loc, opts);
}
flush();
- expectLinks(consumer, {GetParam() ? sameKey : replaced});
- if (GetParam())
+ if (GetParam()) {
+ expectLinks(consumer, {second, sameKey});
EXPECT_THAT(records, Not(Contains(Field(&Record::id, replaced))));
+ } else {
+ expectLinks(consumer, {first, second, sameKey, replaced});
+ }
for (const Record &record : records)
for (remark::RemarkId related : record.related)
EXPECT_THAT(records, Contains(Field(&Record::id, related)));
}
-TEST_P(RemarkHistoryTest, LatestLinkUsesEmissionOrderAndInsertionTime) {
+// Querying before replacement captures r1, but FINAL must remove that link
+// when r2 supersedes r1. It must not redirect the link to r2.
+TEST_P(RemarkHistoryTest, QueryBeforeReplacementPrunesDiscardedTarget) {
engine().enableHistory();
Location loc = UnknownLoc::get(&context);
- auto query = remark::RemarkQuery{}.kind(remark::RemarkKind::RemarkAnalysis);
- remark::RemarkId firstCreated, secondCreated;
+ auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
+ auto query = remark::RemarkQuery::named("Candidate").at(loc);
+ auto r1 = emitMissed(loc, opts);
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(r1));
+ remark::RemarkId r3;
{
- auto pending = remark::analysis(
- loc, remark::RemarkOpts::name("First").category("Opt"));
- firstCreated = pending.getId();
- secondCreated =
- emit(loc, remark::RemarkOpts::name("Second").category("Opt"), true);
+ auto r = remark::missed(
+ loc, remark::RemarkOpts::name("Consumer").category("Opt"));
+ r3 = r.getId();
+ r << remark::relatedTo(query);
}
+ auto r2 = emitMissed(loc, opts);
flush();
- ASSERT_LT(firstCreated.getValue(), secondCreated.getValue());
- auto latest = GetParam() ? secondCreated : firstCreated;
- remark::RemarkId linked;
+ if (GetParam()) {
+ EXPECT_THAT(records,
+ ElementsAre(Field(&Record::id, r3), Field(&Record::id, r2)));
+ expectLinks(r3, {});
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(r2));
+ } else {
+ expectLinks(r3, {r1});
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(r1, r2));
+ }
+}
+
+TEST_P(RemarkHistoryTest, QueryAfterReplacementLinksSurvivingTarget) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
+ auto r1 = emitMissed(loc, opts);
+ auto r2 = emitMissed(loc, opts);
+ auto query = remark::RemarkQuery::named("Candidate").at(loc);
+ if (GetParam())
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(r2));
+ else
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(r1, r2));
+ remark::RemarkId r3;
{
- const auto wrapper = remark::relatedToLatest(query);
- auto r =
- remark::passed(loc, remark::RemarkOpts::name("Linked").category("Opt"));
- linked = r.getId();
- r << wrapper;
- emit(loc, remark::RemarkOpts::name("Later").category("Opt"), true);
- // Even a later emission before this remark is reported cannot retarget it.
- flush();
+ auto r = remark::missed(
+ loc, remark::RemarkOpts::name("Consumer").category("Opt"));
+ r3 = r.getId();
+ r << remark::relatedTo(query);
}
flush();
- expectLinks(linked, {latest});
+ if (GetParam()) {
+ EXPECT_THAT(records,
+ ElementsAre(Field(&Record::id, r3), Field(&Record::id, r2)));
+ expectLinks(r3, {r2});
+ } else {
+ expectLinks(r3, {r1, r2});
+ }
+}
- const auto savedWrapper = remark::relatedToLatest(query);
- engine().clearHistory();
- remark::RemarkId noMatch;
+TEST_P(RemarkHistoryTest, QueryLinksBeforeFinalization) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto target =
+ emitMissed(loc, remark::RemarkOpts::name("Candidate").category("Opt"));
+ remark::RemarkId consumer;
{
- auto r = remark::passed(
- loc, remark::RemarkOpts::name("NoMatch").category("Opt"));
- noMatch = r.getId();
- r << savedWrapper;
+ auto r = remark::missed(
+ loc, remark::RemarkOpts::name("Consumer").category("Opt"));
+ consumer = r.getId();
+ r << remark::relatedTo(remark::RemarkQuery::named("Candidate"));
}
+ if (GetParam())
+ EXPECT_THAT(records, IsEmpty());
flush();
- expectLinks(noMatch, {});
+ expectLinks(consumer, {target});
+ EXPECT_EQ(records.size(), 2u);
+}
+
+TEST_P(RemarkHistoryTest, EarlierBatchLinksSurviveHistoryReset) {
+ Location loc = UnknownLoc::get(&context);
+ for (bool disable : {false, true}) {
+ SCOPED_TRACE(disable);
+ // Also track emitted IDs when query history is disabled at emission time.
+ engine().enableHistory(!disable);
+ auto target = emit(loc, remark::RemarkOpts::name("Target").category("Opt"));
+ flush();
+ if (disable)
+ engine().enableHistory();
+ else
+ engine().clearHistory();
+ EXPECT_THAT(engine().findRemarks(remark::RemarkQuery{}), IsEmpty());
+ engine().enableHistory(false);
+ auto consumer = emit(
+ loc,
+ remark::RemarkOpts::name("Consumer").category("Opt").relatedTo(target));
+ flush();
+ expectLinks(consumer, {target});
+ EXPECT_EQ(llvm::count_if(records,
+ [&](const Record &r) { return r.id == target; }),
+ 1);
+ }
+}
- auto fresh =
- emit(loc, remark::RemarkOpts::name("Fresh").category("Opt"), true);
+TEST_P(RemarkHistoryTest, ExplicitLinksPruneDiscardedAndUnknownTargets) {
+ Location loc = UnknownLoc::get(&context);
+ auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
+ auto discarded = emit(loc, opts);
+ auto survivor = emit(loc, opts);
+ remark::RemarkId unknown(1000000);
+ remark::detail::OptRemarkPass consumer(
+ loc, remark::RemarkOpts::name("Consumer").category("Opt"));
+ auto consumerId = engine().generateRemarkId();
+ consumer.setId(consumerId);
+ consumer.addRelatedRemark(discarded);
+ consumer.addRelatedRemark(survivor);
+ consumer.addRelatedRemark(unknown);
+ engine().report(std::move(consumer));
flush();
- remark::RemarkId afterReset;
+ if (GetParam()) {
+ expectLinks(consumerId, {survivor});
+ EXPECT_EQ(records.size(), 2u);
+ EXPECT_THAT(records, Not(Contains(Field(&Record::id, discarded))));
+ } else {
+ expectLinks(consumerId, {discarded, survivor, unknown});
+ }
+}
+
+TEST_P(RemarkHistoryTest, ExplicitLinksDoNotWaitForFutureBatches) {
+ Location loc = UnknownLoc::get(&context);
+ remark::RemarkId consumer, futureId;
{
- auto r = remark::passed(
- loc, remark::RemarkOpts::name("AfterReset").category("Opt"));
- afterReset = r.getId();
- r << savedWrapper;
+ auto future = remark::analysis(
+ loc, remark::RemarkOpts::name("Future").category("Opt"));
+ futureId = future.getId();
+ consumer = emit(
+ loc,
+ remark::RemarkOpts::name("Consumer").category("Opt").relatedTo(future));
+ flush();
+ if (GetParam())
+ expectLinks(consumer, {});
+ else
+ expectLinks(consumer, {futureId});
}
flush();
- expectLinks(afterReset, {fresh});
+ EXPECT_EQ(records.size(), 2u);
+ EXPECT_THAT(records, Contains(Field(&Record::id, futureId)));
+ if (GetParam())
+ expectLinks(consumer, {});
}
-TEST_P(RemarkHistoryTest, RepeatedDeliveryRetainsFirstEmissionOrder) {
+TEST_P(RemarkHistoryTest, TransitiveLinksAndCyclesEmitEachSurvivorOnce) {
+ Location loc = UnknownLoc::get(&context);
+ auto leaf = emit(loc, remark::RemarkOpts::name("Leaf").category("Opt"));
+ auto middle = emit(
+ loc, remark::RemarkOpts::name("Middle").category("Opt").relatedTo(leaf));
+ auto root = emit(
+ loc, remark::RemarkOpts::name("Root").category("Opt").relatedTo(middle));
+ auto cycleA = engine().generateRemarkId();
+ auto cycleB = engine().generateRemarkId();
+ auto self = engine().generateRemarkId();
+ auto report = [&](remark::RemarkId id, StringRef name,
+ remark::RemarkId related) {
+ remark::detail::OptRemarkPass r(
+ loc, remark::RemarkOpts::name(name).category("Opt").relatedTo(related));
+ r.setId(id);
+ engine().report(std::move(r));
+ };
+ report(cycleA, "CycleA", cycleB);
+ report(cycleB, "CycleB", cycleA);
+ report(self, "Self", self);
+ flush();
+ ASSERT_EQ(records.size(), 6u);
+ for (auto id : {leaf, middle, root, cycleA, cycleB, self})
+ EXPECT_EQ(
+ llvm::count_if(records, [&](const Record &r) { return r.id == id; }),
+ 1);
+ expectLinks(middle, {leaf});
+ expectLinks(root, {middle});
+ expectLinks(cycleA, {cycleB});
+ expectLinks(cycleB, {cycleA});
+ expectLinks(self, {self});
+ if (GetParam())
+ EXPECT_THAT(
+ records,
+ ElementsAre(Field(&Record::id, root), Field(&Record::id, middle),
+ Field(&Record::id, leaf), Field(&Record::id, cycleA),
+ Field(&Record::id, cycleB), Field(&Record::id, self)));
+ flush();
+ EXPECT_EQ(records.size(), 6u);
+}
+
+TEST_P(RemarkHistoryTest, SharedTargetsEmitOnceAndQueriesUseCreationOrder) {
engine().enableHistory();
Location loc = UnknownLoc::get(&context);
auto child =
@@ -1294,39 +1430,42 @@ TEST_P(RemarkHistoryTest, RepeatedDeliveryRetainsFirstEmissionOrder) {
true);
flush();
auto query = remark::RemarkQuery{};
- if (GetParam()) {
- EXPECT_THAT(engine().findRemarks(query),
- ElementsAre(firstParent, child, secondParent));
- EXPECT_EQ(
- llvm::count_if(records, [&](const Record &r) { return r.id == child; }),
- 2);
- } else {
- EXPECT_THAT(engine().findRemarks(query),
- ElementsAre(child, firstParent, secondParent));
- }
-
- // FINAL delivered Child again after SecondParent; it is still the older
- // match.
+ EXPECT_THAT(engine().findRemarks(query),
+ ElementsAre(child, firstParent, secondParent));
+ EXPECT_EQ(
+ llvm::count_if(records, [&](const Record &r) { return r.id == child; }),
+ 1);
+ EXPECT_EQ(records.size(), 3u);
+ expectLinks(firstParent, {child});
+ expectLinks(secondParent, {child});
+
+ // Grouping can emit a parent first; queries still return creation order.
auto analyses = query.kind(remark::RemarkKind::RemarkAnalysis);
EXPECT_THAT(engine().findRemarks(analyses), ElementsAre(child, secondParent));
remark::RemarkId linked;
{
auto r =
- remark::passed(loc, remark::RemarkOpts::name("Latest").category("Opt"));
+ remark::passed(loc, remark::RemarkOpts::name("Linked").category("Opt"));
linked = r.getId();
- r << remark::relatedToLatest(analyses);
+ r << remark::relatedTo(analyses);
}
flush();
- expectLinks(linked, {secondParent});
+ expectLinks(linked, {child, secondParent});
}
-TEST_P(RemarkHistoryTest, StreamerCanQueryCurrentReport) {
+TEST_P(RemarkHistoryTest, StreamerCanQueryCurrentReportAndActiveDrain) {
engine().enableHistory();
queryInCallback = true;
- auto id = emit(UnknownLoc::get(&context),
- remark::RemarkOpts::name("Candidate").category("Opt"));
+ Location loc = UnknownLoc::get(&context);
+ auto first = emit(loc, remark::RemarkOpts::name("First").category("Opt"));
+ auto second = emit(loc, remark::RemarkOpts::name("Second").category("Opt"));
flush();
- EXPECT_THAT(callbackHistory, ElementsAre(ElementsAre(id)));
+ if (GetParam())
+ EXPECT_THAT(callbackHistory, ElementsAre(ElementsAre(first, second),
+ ElementsAre(first, second)));
+ else
+ EXPECT_THAT(callbackHistory,
+ ElementsAre(ElementsAre(first), ElementsAre(first, second)));
}
INSTANTIATE_TEST_SUITE_P(EmittingPolicies, RemarkHistoryTest, Bool(),
>From 616681711a9a8faf19106a91f672938bdbfc0c46 Mon Sep 17 00:00:00 2001
From: Guray Ozen <gozen at nvidia.com>
Date: Tue, 3 Nov 2026 14:12:21 +0100
Subject: [PATCH 4/4] [mlir] Harden remark history links, tests, and docs
- Link each query target once: relatedTo(query) skips targets that are
already linked explicitly or by an earlier query.
- Declare the engine history before the streamer and the policy so that a
policy reporting from its destructor can still record history.
- Keep the relative order of repeated keys when printing remark arguments,
matching the serialized form.
- Compare location and kind before the string fields when matching queries;
fill the RemarkOpts query directly; add missing includes.
- Tests: reach the engine through a cached pointer, since the context's
pointer is null during the destructor drain; add hooks for reporting from
a streamer callback; cover self-replacement, function-scoped replacement,
link dedup, Failure and Passed identities at one site, reports from a
callback mid-drain, queries during the destructor drain, and concurrent
reports under Final; assert the cross-batch RelatedTo link.
- Docs: state the Final pruning rule, the function-blind identity,
destructor-driven drains, emitted-ID retention, query cost, the reserved
RemarkId and RelatedTo keys, and the meaning of "emitted" once each; fix
the stale lit comments.
Co-Authored-By: Claude Fable 5.1 <noreply at anthropic.com>
---
mlir/docs/Remarks.md | 117 +++++++-----
mlir/include/mlir/IR/Remarks.h | 75 +++++---
mlir/lib/IR/Remarks.cpp | 47 +++--
mlir/test/Pass/remark-final.mlir | 11 +-
mlir/test/Pass/remarks.mlir | 6 +-
mlir/unittests/IR/RemarkTest.cpp | 312 +++++++++++++++++++++++++------
6 files changed, 413 insertions(+), 155 deletions(-)
diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md
index e18b88124bfadc..4c315c32c04bbf 100644
--- a/mlir/docs/Remarks.md
+++ b/mlir/docs/Remarks.md
@@ -204,8 +204,10 @@ Emits **all** remarks unconditionally.
Stores remarks until `finalize()` is called and keeps only the **last reported**
remark for each identity: location, remark name, combined category (including
-its subcategory), and remark kind. Function names are not part of this identity.
-Remarks of different kinds do not replace one another.
+its subcategory), and remark kind. Function names are not part of this identity,
+so remarks that differ only in their function name replace each other. Remarks
+of different kinds do not replace one another: a `Failure` followed by a
+`Passed` remark at the same location are two identities, and both are emitted.
**Example:** Only the second missed remark is emitted:
@@ -221,15 +223,18 @@ remark::missed(loc, opts) << "Cannot unroll: code size limit exceeded";
`finalize()` drains the stored remarks, visits top-level remarks in creation
order, and groups related surviving remarks after the remarks that reference
-them. It follows links transitively and emits each survivor once, including
-when links form a cycle. Calling `finalize()` again emits only remarks reported
-since the previous call.
-
-Before emission, the policy removes links to discarded or unknown IDs, including
-their serialized `RelatedTo` arguments. It does not redirect a link to a
-replacement remark or discard a remark because its target was discarded. Links
-to targets emitted by an earlier `finalize()` remain valid without emitting
-those targets again.
+them. It follows links transitively and emits each survivor once; members of a
+cycle that no top-level remark reaches follow all other remarks. Calling
+`finalize()` again emits only remarks reported since the previous call.
+
+Before emission, the policy keeps a link only if its target is in the same
+batch or was emitted by an earlier `finalize()`. Every other link is removed
+together with its serialized `RelatedTo` argument: links to replaced remarks,
+to unknown IDs, and to remarks still in flight when `finalize()` runs. A link
+is never redirected to the replacement remark, and a remark is never discarded
+because its target was. Targets emitted by an earlier `finalize()` are not
+emitted again; the set of emitted IDs that makes this work is kept for the
+lifetime of the policy, even while history is disabled.
You can also implement custom policies by inheriting from the policy interface.
@@ -267,14 +272,19 @@ if (engine->isRemarkEnabled(remark::RemarkKind::RemarkPassed, "Vectorizer"))
History is opt-in. When enabled, queries search both previously emitted remarks
and the current pending remarks held by `Final`. A pending remark is searchable
as soon as it is reported; if another report replaces it, subsequent queries
-see the replacement. The engine stores emitted IDs and identifying metadata,
-with interned strings, but not messages or arguments. `Final` owns its pending
-full remarks independently of this metadata cache.
-
-Each matching ID is returned once, in creation-ID order. Previously emitted
-targets remain searchable across batches until history is cleared or disabled.
-Enabling history also exposes existing pending `Final` remarks, but cannot
-recover earlier emissions or engine-filtered remarks.
+see the replacement. "Emitted" means selected by the policy and delivered by
+the engine: the engine records the ID and identifying metadata, with interned
+strings but without messages or arguments, before the streamer or the
+diagnostic printer runs, so a streamer that drops a remark leaves it
+searchable. `Final` owns its pending full remarks independently of this
+metadata cache.
+
+Each matching ID is returned once, in creation-ID order. Each query scans every
+pending `Final` remark and every history entry, so a query per remark costs
+time proportional to the number of earlier remarks; pass an ID you already
+hold with `RemarkOpts::relatedTo(id)` instead. Enabling history also exposes existing
+pending `Final` remarks, but cannot recover earlier emissions or
+engine-filtered remarks.
`RemarkQuery` matches name, base category, subcategory, function, kind, and
complete location, including wrappers. Specified fields use exact AND matching;
@@ -284,6 +294,8 @@ categories:
```c++
auto *engine = context.getRemarkEngine();
+if (!engine)
+ return; // No remark engine is configured.
engine->enableHistory();
auto analysisOpts = remark::RemarkOpts::name("VectorSizeInference")
.category("Vectorizer")
@@ -296,49 +308,62 @@ auto query = remark::RemarkQuery::named("VectorSizeInference")
.function("kernel")
.kind(remark::RemarkKind::RemarkAnalysis)
.at(loc);
-// Matching IDs are returned in creation-ID order.
-SmallVector<remark::RemarkId> matches = engine->findRemarks(query);
remark::missed(loc, remark::RemarkOpts::name("Vectorization")
.category("Vectorizer")
.function("kernel"))
<< remark::relatedTo(query);
```
-`relatedTo(query)` links all matches and resolves at `<<`; no match or disabled
-history adds no links. It does not rerun the query when the remark is emitted.
-Known IDs can use `RemarkOpts::relatedTo(id)`. Under `Final`, links to targets
-that are subsequently replaced are removed during finalization. A query result
-therefore describes the current candidates, not a guarantee of eventual
-emission.
+`findRemarks(query)` returns the matching IDs. `relatedTo(query)` resolves at
+`<<` and links every match once; no match or disabled history adds no links,
+and a target that is already linked is not linked again. It does not rerun the
+query when the remark is emitted. Known IDs can use `RemarkOpts::relatedTo(id)`.
+Under `Final`, links to targets that are replaced before finalization are
+removed as described in the policy section above. A query result therefore
+describes the current candidates, not a guarantee of eventual emission.
For example, suppose `r1` and `r2` have the same Final identity, `r3` has a
different identity, and all three are reported before finalization:
| Report and query order | Final output |
|------------------------|--------------|
-| Report `r1`; query and attach `r3 → r1`; report replacement `r2` | `r2` and `r3`, with the link removed |
-| Report `r1`; report replacement `r2`; query and attach `r3 → r2` | `r2` and `r3`, with the link preserved |
-
-Serialized links repeat the `RelatedTo` key, so consumers must preserve
-duplicates. Matching does not establish causality, operation identity, or
-continued validity after IR changes.
+| Report `r1`; query and attach `r3 → r1`; report replacement `r2` | `r3`, then `r2`; link removed |
+| Report `r1`; report replacement `r2`; query and attach `r3 → r2` | `r3`, then `r2`; link preserved |
+
+Serialized links repeat the `RelatedTo` key once per link, so consumers must
+preserve repeated keys; query-based links add each target once, while
+`Remark::addRelatedRemark` adds a link on every call. The engine writes the
+`RemarkId` and `RelatedTo`
+arguments itself; do not use these keys for producer arguments. A
+`function()`-scoped query narrows the match but does not protect the link: the
+function is not part of the `Final` identity, so a same-identity report from
+another function, or the linking remark itself, can replace the target.
+Matching does not establish causality, operation identity, or continued
+validity after IR changes.
Queries own their criteria strings; queries with locations must not outlive the
-locations' `MLIRContext`.
-History access is synchronized, including queries from streamer callbacks that
-see the current event. This does not make custom policies or streamers
-thread-safe. With a parallel pass manager, cross-function query membership and
-ID creation order depend on timing; scope queries to a function and sequence
-producers or use explicit barriers.
+locations' `MLIRContext`. The engine synchronizes history access and the
+`Final` pending set, so queries may run from any thread and from streamer
+callbacks, which see the current event and the rest of the batch being
+drained. The engine does not serialize reports: under `All`, the streamer and
+the diagnostic printer run on the reporting thread. With a parallel pass
+manager, cross-function query membership and ID creation order depend on
+timing; scope queries to a function and sequence producers before consumers.
+A streamer callback may also report remarks; under `Final` they join the next
+batch. During drains driven by engine destruction, for example `~MLIRContext`,
+`MLIRContext::getRemarkEngine()` already returns null, so the helpers find no
+engine, and a remark reported through a retained engine pointer joins a batch
+that is never drained; neither is emitted. A callback that needs to query
+there must retain a pointer to the engine.
Emitted metadata is retained until `clearHistory()`, `enableHistory(false)`, or
-engine destruction. Clearing keeps recording, and pending `Final` remarks remain
-searchable. Disabling stops recording and makes queries return no matches.
-Neither operation resets IDs, changes existing links, or cancels pending
-`Final` remarks. Repeated enabling preserves entries. `Final` independently
-remembers which IDs it emitted so that existing links to earlier batches remain
-valid even after history is cleared or disabled. Repeated broad all-match
-queries can produce quadratic numbers of links.
+engine destruction. Clearing keeps recording; disabling stops recording and
+makes queries return no matches. Neither operation resets IDs, changes existing
+links, or affects pending `Final` remarks. Repeated enabling preserves
+entries. The emitted-ID set
+that `Final` keeps for its links, described in the policy section above, is
+not affected either and is released only by replacing the engine. Repeated
+broad all-match queries can produce quadratic numbers of links.
***
diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 2c5bfa3befa669..701512287018c8 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -138,7 +138,9 @@ struct RemarkOpts {
}
/// Link this remark to a related remark that is still in flight.
- /// Extracts the ID from the InFlightRemark.
+ /// Extracts the ID from the InFlightRemark. Under RemarkEmittingPolicyFinal
+ /// the link survives only if the target is reported before the batch holding
+ /// this remark is finalized; see RemarkEmittingPolicyFinal::finalize().
inline RemarkOpts relatedTo(const detail::InFlightRemark &r) const;
};
@@ -148,7 +150,8 @@ struct RemarkOpts {
/// must not outlive it. A location match does not imply operation identity.
class RemarkQuery {
public:
- /// Construct a query matching the given remark name.
+ /// Construct a query matching the given remark name. Spelled `named` because
+ /// a static `name` cannot overload the `name` builders below.
[[nodiscard]] static RemarkQuery named(StringRef value) {
return RemarkQuery().name(value);
}
@@ -212,6 +215,7 @@ namespace mlir::remark::detail {
// Remark Base Class
//===----------------------------------------------------------------------===//
class Remark {
+
public:
Remark(RemarkKind remarkKind, DiagnosticSeverity severity, Location loc,
RemarkOpts opts)
@@ -322,7 +326,9 @@ class Remark {
/// Get this remark's unique ID (0 if not assigned).
RemarkId getId() const { return id; }
- /// Set this remark's unique ID. Also adds it as an Arg for serialization.
+ /// Set this remark's unique ID. Also adds it as a "RemarkId" Arg for
+ /// serialization. The "RemarkId" and "RelatedTo" keys are reserved for the
+ /// engine; producer arguments must not use them.
void setId(RemarkId newId) {
id = newId;
if (id)
@@ -332,8 +338,9 @@ class Remark {
/// Get the list of related remark IDs.
ArrayRef<RemarkId> getRelatedRemarkIds() const { return relatedRemarks; }
- /// Add a reference to a related remark. Also adds it as an Arg for
- /// serialization.
+ /// Add a reference to a related remark. Also adds it as a "RelatedTo" Arg for
+ /// serialization. Every call adds a link; remark::relatedTo(RemarkQuery)
+ /// skips targets that are already linked.
void addRelatedRemark(RemarkId relatedId) {
if (relatedId) {
relatedRemarks.push_back(relatedId);
@@ -341,7 +348,8 @@ class Remark {
}
}
- /// Remove links rejected by the predicate, including their serialized args.
+ /// Remove links rejected by the predicate, including the serialized
+ /// "RelatedTo" args that carry their IDs.
void retainRelatedRemarks(llvm::function_ref<bool(RemarkId)> shouldKeep);
/// Check if this remark has any related remarks.
@@ -571,9 +579,10 @@ class RemarkEmittingPolicyBase {
return {};
}
- /// Visit remarks retained for future emission. The callback must not reenter
- /// the policy or retain references to the remarks. Policies may hold a lock
- /// while invoking it. The default policy has no pending remarks.
+ /// Visit remarks retained for future emission. RemarkEngine::findRemarks
+ /// calls this hook, so a policy must not call findRemarks while holding the
+ /// lock it takes here. The callback must not reenter the policy or retain
+ /// references to the remarks. The default implementation visits nothing.
virtual void forEachPendingRemark(
llvm::function_ref<void(const Remark &)> callback) const {}
};
@@ -593,6 +602,13 @@ class RemarkEngine {
std::optional<llvm::Regex> analysisFilter;
/// The category for failed optimization remarks.
std::optional<llvm::Regex> failedFilter;
+ /// History records metadata for remarks selected for emission by the policy.
+ /// Its mutex has a stable lifetime; retained storage is allocated on demand.
+ /// Declared before the streamer and the policy so that a custom policy whose
+ /// destructor still reports can record history safely.
+ struct RemarkHistory;
+ std::unique_ptr<RemarkHistory> history;
+ std::atomic<bool> historyEnabled{false};
/// The MLIR remark streamer that will be used to emit the remarks.
std::unique_ptr<MLIRRemarkStreamerBase> remarkStreamer;
/// The MLIR remark policy that will be used to emit the remarks.
@@ -602,12 +618,6 @@ class RemarkEngine {
/// Atomic counter for generating unique remark IDs.
std::atomic<uint64_t> nextRemarkId{1};
- /// History records metadata for remarks selected for emission by the policy.
- /// Its mutex has a stable lifetime; retained storage is allocated on demand.
- struct RemarkHistory;
- std::unique_ptr<RemarkHistory> history;
- std::atomic<bool> historyEnabled{false};
-
/// Emit a remark using the given maker function, which should return
/// a Remark instance. The remark will be emitted using the main
/// remark streamer.
@@ -708,15 +718,16 @@ class RemarkEngine {
/// Find distinct emitted and pending remarks matching every specified
/// criterion, in ID creation order. Final exposes its current survivors;
/// replacement removes the old pending event from subsequent queries.
- /// Returns empty when history is disabled. Finalization may add emitted
- /// metadata but does not clear history. A streamer callback can query the
- /// current event and the remaining pending remarks. History access is
- /// synchronized; this does not make custom policies or streamers thread-safe.
+ /// Returns empty when history is disabled. Each call scans every pending
+ /// remark and every history entry. History access and the Final pending set
+ /// are synchronized, so a streamer callback can query the current event and
+ /// the remaining pending remarks; the engine does not serialize reports.
///
/// Cross-function queries during parallel pass execution have
/// timing-dependent results and ordering. Scope queries with function() and
/// sequence producers before consumers when deterministic relationships are
- /// required.
+ /// required. See "Searching and Linking Remarks" in docs/Remarks.md for the
+ /// Final pruning and lifetime caveats.
SmallVector<RemarkId> findRemarks(const RemarkQuery &query) const;
/// Search using nonempty option fields as exact-match criteria. The related
@@ -769,9 +780,10 @@ mlir::remark::RemarkOpts::relatedTo(const detail::InFlightRemark &r) const {
namespace mlir::remark {
/// Link to all remarks matching the query when this helper is inserted into an
/// in-flight remark. History must be enabled. No matches (including disabled
-/// history) adds no links. Final removes links to targets discarded before
-/// emission; links are never redirected to their replacements. Matching
-/// indicates a relationship, not causality or operation identity.
+/// history) adds no links; a target that is already linked is not linked
+/// again. Final removes links to targets discarded before emission; links are
+/// never redirected to their replacements. Matching indicates a relationship,
+/// not causality or operation identity.
inline detail::RelatedRemarkQuery relatedTo(RemarkQuery query) {
return {std::move(query)};
}
@@ -793,9 +805,10 @@ class RemarkEmittingPolicyAll : public detail::RemarkEmittingPolicyBase {
};
/// Policy that emits only the last remark reported for each identity, see
-/// DenseMapInfo<Remark>. Remarks are stored until finalize(), which emits them
-/// in creation order, grouping related remarks after their parents. Links to
-/// discarded targets are removed; they do not invalidate their source remark.
+/// DenseMapInfo<Remark>. Remarks are stored until finalize(), which visits
+/// top-level remarks in creation order and emits each one's related survivors
+/// right after it; remaining cycle members follow all roots. See finalize()
+/// for link pruning.
class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
private:
struct PendingRemarks;
@@ -807,13 +820,17 @@ class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
void reportRemark(const detail::Remark &remark) override;
+ /// Visit the pending remarks and the batch currently being emitted by
+ /// finalize().
void forEachPendingRemark(
llvm::function_ref<void(const detail::Remark &)> callback) const override;
/// Emit each survivor once, following related remarks transitively. Links
- /// may reference current survivors or previously emitted IDs. Other links
- /// are removed. Previously emitted targets are not repeated. Cycles do not
- /// suppress output. A later call emits only newly reported remarks.
+ /// may reference current survivors or previously emitted IDs. Other links,
+ /// including links to targets still in flight, are removed. Previously
+ /// emitted targets are not repeated. Cycles do not suppress output. A later
+ /// call emits only newly reported remarks. The set of emitted IDs is kept for
+ /// the lifetime of the policy.
void finalize() override;
};
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index 8164000fd83869..b16faceb9fab25 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -12,7 +12,9 @@
#include "mlir/IR/Diagnostics.h"
#include "mlir/IR/Value.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/StringSaver.h"
@@ -68,8 +70,10 @@ static void printArgs(llvm::raw_ostream &os, llvm::ArrayRef<Remark::Arg> args) {
if (args.empty())
return;
+ // Keep the relative order of repeated keys, such as several RelatedTo args,
+ // as in the serialized remark.
llvm::SmallVector<Remark::Arg, 8> sorted(args.begin(), args.end());
- llvm::sort(sorted, [](const Remark::Arg &a, const Remark::Arg &b) {
+ llvm::stable_sort(sorted, [](const Remark::Arg &a, const Remark::Arg &b) {
return a.key < b.key;
});
@@ -189,8 +193,15 @@ llvm::remarks::Remark Remark::generateRemark() const {
//===----------------------------------------------------------------------===//
InFlightRemark &InFlightRemark::operator<<(const RelatedRemarkQuery &related) {
- if (remark && owner)
- for (RemarkId id : owner->findRemarks(related.query))
+ if (!remark || !owner)
+ return *this;
+ // Link each target once, even if an explicit link or an earlier query
+ // already added it.
+ llvm::SmallDenseSet<uint64_t> linked;
+ for (RemarkId id : remark->getRelatedRemarkIds())
+ linked.insert(id.getValue());
+ for (RemarkId id : owner->findRemarks(related.query))
+ if (linked.insert(id.getValue()).second)
remark->addRelatedRemark(id);
return *this;
}
@@ -233,7 +244,8 @@ struct RemarkEngine::RemarkHistory {
llvm::BumpPtrAllocator allocator;
llvm::UniqueStringSaver strings{allocator};
std::vector<Entry> entries;
- llvm::DenseSet<uint64_t> emittedIds;
+ // IDs with an entry; distinct from the Final policy's emitted-ID set.
+ llvm::DenseSet<uint64_t> recordedIds;
};
// Keep the mutex alive for the engine's lifetime so disabling history cannot
@@ -270,13 +282,14 @@ RemarkEngine::findRemarks(const RemarkQuery &query) const {
auto matches = [](const auto &filter, const auto &value) {
return !filter || *filter == value;
};
+ // Pointer and enum comparisons first: they reject most entries cheaply.
auto matchesQuery = [&](const RemarkHistory::Entry &entry) {
- return matches(query.remarkName, entry.name) &&
+ return matches(query.location, entry.location) &&
+ matches(query.remarkKind, entry.kind) &&
+ matches(query.remarkName, entry.name) &&
matches(query.categoryName, entry.category) &&
matches(query.subCategoryName, entry.subCategory) &&
- matches(query.functionName, entry.function) &&
- matches(query.remarkKind, entry.kind) &&
- matches(query.location, entry.location);
+ matches(query.functionName, entry.function);
};
// Read pending remarks before emitted history: a remark delivered between
// these snapshots can appear in both, but cannot disappear between them.
@@ -289,6 +302,7 @@ RemarkEngine::findRemarks(const RemarkQuery &query) const {
{
std::scoped_lock<std::mutex> lock(history->mutex);
+ // History was disabled concurrently: report the disabled result.
if (!history->storage)
return {};
for (const RemarkHistory::Entry &entry : history->storage->entries)
@@ -307,15 +321,14 @@ RemarkEngine::findRemarks(const RemarkOpts &opts,
std::optional<RemarkKind> kind) const {
RemarkQuery query;
if (!opts.remarkName.empty())
- query = query.name(opts.remarkName);
+ query.remarkName = opts.remarkName.str();
if (!opts.categoryName.empty())
- query = query.category(opts.categoryName);
+ query.categoryName = opts.categoryName.str();
if (!opts.subCategoryName.empty())
- query = query.subCategory(opts.subCategoryName);
+ query.subCategoryName = opts.subCategoryName.str();
if (!opts.functionName.empty())
- query = query.function(opts.functionName);
- if (kind)
- query = query.kind(*kind);
+ query.functionName = opts.functionName.str();
+ query.remarkKind = kind;
return findRemarks(query);
}
@@ -404,7 +417,7 @@ void RemarkEngine::reportImpl(const Remark &remark) {
auto &storage = *history->storage;
// Custom policies can deliver an event more than once. Retain one history
// entry per event so a query does not produce duplicate links.
- if (storage.emittedIds.insert(remark.getId().getValue()).second)
+ if (storage.recordedIds.insert(remark.getId().getValue()).second)
storage.entries.push_back(
RemarkHistory::Entry::from(remark, storage.strings));
}
@@ -487,8 +500,8 @@ buildFilter(const mlir::remark::RemarkCategories &cats,
RemarkEngine::RemarkEngine(bool printAsEmitRemarks,
const RemarkCategories &cats)
- : printAsEmitRemarks(printAsEmitRemarks),
- history(std::make_unique<RemarkHistory>()) {
+ : history(std::make_unique<RemarkHistory>()),
+ printAsEmitRemarks(printAsEmitRemarks) {
if (cats.passed)
passedFilter = buildFilter(cats, cats.passed);
if (cats.missed)
diff --git a/mlir/test/Pass/remark-final.mlir b/mlir/test/Pass/remark-final.mlir
index d406de295c5513..6b76a51a4e7ccc 100644
--- a/mlir/test/Pass/remark-final.mlir
+++ b/mlir/test/Pass/remark-final.mlir
@@ -5,11 +5,12 @@ module @foo {
"test.op"() : () -> ()
}
-// Remarks come out in creation order. The two passed remarks in
-// "category-1-passed" share an identity, so only the second survives. mlir-opt
-// calls finalize() explicitly and the engine destructor calls it again. The
-// second call must not emit the remarks a second time. --implicit-check-not
-// pins the number of "remark:" lines and of YAML records to five.
+// Top-level remarks come out in creation order, each followed by the remarks
+// linked to it. The two passed remarks in "category-1-passed" share an
+// identity, so only the second survives. mlir-opt calls finalize() explicitly
+// and the engine destructor calls it again. The second call must not emit the
+// remarks a second time. --implicit-check-not pins the number of "remark:"
+// lines and of YAML records to five.
// CHECK: remark: [Passed] test-remark | Category:category-1-passed |{{.*}}Remark="This is a test passed remark",
// CHECK: remark: [Failure] test-remark | Category:category-2-failed
diff --git a/mlir/test/Pass/remarks.mlir b/mlir/test/Pass/remarks.mlir
index f99a98e32ed1fa..569f2fbd59307b 100644
--- a/mlir/test/Pass/remarks.mlir
+++ b/mlir/test/Pass/remarks.mlir
@@ -28,9 +28,9 @@ module @foo {
// CHECK-ALL1-NOT: remark: [Failure]
// CHECK-ALL1-NOT: remark: [Analysis]
-// Test remark linking via query-based API with PolicyFinal: the passed remark
-// finds the analysis remark by searching postponedRemarks. Related (child)
-// remarks are grouped after their parent.
+// Test remark linking with PolicyFinal: the passed remark links to the analysis
+// remark by ID (RemarkOpts::relatedTo(InFlightRemark)). Related (child) remarks
+// are grouped after their parent.
// CHECK-LINK: remark: [Passed]{{.*}}Category:category-link |{{.*}}RelatedTo=
// CHECK-LINK-SAME: Remark="Optimization enabled by analysis"
// CHECK-LINK-SAME: RemarkId=
diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp
index d7a55dcd6316ad..15cb2521262253 100644
--- a/mlir/unittests/IR/RemarkTest.cpp
+++ b/mlir/unittests/IR/RemarkTest.cpp
@@ -13,6 +13,7 @@
#include "mlir/IR/Remarks.h"
#include "mlir/Remark/RemarkStreamer.h"
#include "mlir/Support/TypeID.h"
+#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringRef.h"
@@ -26,6 +27,7 @@
#include "gtest/gtest.h"
#include <functional>
#include <optional>
+#include <thread>
#include <vector>
using namespace mlir;
@@ -414,16 +416,21 @@ TEST(Remark, TestRemarkFinal) {
}
/// Streamer that records "<remark name>: <Remark arg>" for every remark it
-/// receives, independent of how Remark::print formats its output.
+/// receives, followed by " RelatedTo=<id>" for each link, independent of how
+/// Remark::print formats its output.
class RecordingStreamer : public remark::detail::MLIRRemarkStreamerBase {
public:
explicit RecordingStreamer(std::vector<std::string> &out) : out(out) {}
void streamOptimizationRemark(const remark::detail::Remark &remark) override {
std::string entry = remark.getRemarkName().str();
- for (const remark::detail::Remark::Arg &arg : remark.getArgs())
+ std::string links;
+ for (const remark::detail::Remark::Arg &arg : remark.getArgs()) {
if (arg.key == "Remark")
entry += ": " + arg.val;
- out.push_back(std::move(entry));
+ else if (arg.key == "RelatedTo")
+ links += " RelatedTo=" + arg.val;
+ }
+ out.push_back(entry + links);
}
private:
@@ -499,13 +506,13 @@ TEST(Remark, TestRemarkFinalDrains) {
// again when the parent is drained.
TEST(Remark, TestRemarkFinalLinkAcrossDrains) {
std::vector<std::string> emitted;
+ remark::RemarkId analysisId;
{
MLIRContext context;
ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
Location loc = FileLineColLoc::get(&context, "test.cpp", 1, 5);
auto *policy = context.getRemarkEngine()->getRemarkEmittingPolicy();
- remark::RemarkId analysisId;
{
// The remark is reported when the InFlightRemark goes out of scope.
auto analysis = remark::analysis(
@@ -521,8 +528,9 @@ TEST(Remark, TestRemarkFinalLinkAcrossDrains) {
.relatedTo(analysisId))
<< "unrolled";
}
- EXPECT_THAT(emitted,
- ElementsAre("Analysis: trip count 128", "Unroller: unrolled"));
+ EXPECT_THAT(emitted, ElementsAre("Analysis: trip count 128",
+ "Unroller: unrolled RelatedTo=" +
+ std::to_string(analysisId.getValue())));
}
TEST(Remark, TestArgWithAttribute) {
@@ -820,12 +828,14 @@ class RemarkHistoryTest : public TestWithParam<bool> {
};
void SetUp() override {
+ // Also match an empty category so that RemarkOpts{} is accepted.
remark::RemarkCategories categories;
categories.passed = "(Opt.*)?";
categories.missed = "(Opt.*)?";
categories.analysis = "(Opt.*)?";
+ categories.failed = "(Opt.*)?";
std::unique_ptr<remark::detail::RemarkEmittingPolicyBase> policy;
- if (GetParam())
+ if (isFinal())
policy = std::make_unique<remark::RemarkEmittingPolicyFinal>();
else
policy = std::make_unique<remark::RemarkEmittingPolicyAll>();
@@ -839,20 +849,33 @@ class RemarkHistoryTest : public TestWithParam<bool> {
records.push_back(std::move(record));
if (queryInCallback)
callbackHistory.push_back(engine().findRemarks(remark::RemarkQuery{}));
+ if (onRecord)
+ onRecord(r);
});
ASSERT_TRUE(succeeded(remark::enableOptimizationRemarks(
context, std::move(streamer), std::move(policy), categories)));
+ // ~MLIRContext nulls its engine pointer before the engine destructor drains
+ // the Final policy, so callbacks must reach the engine through this pointer
+ // rather than through the context.
+ remarkEngine = context.getRemarkEngine();
+ ASSERT_TRUE(remarkEngine);
ASSERT_TRUE(engine().isPassedOptRemarkEnabled("Opt"));
ASSERT_TRUE(engine().isAnalysisOptRemarkEnabled("Opt"));
ASSERT_TRUE(engine().isPassedOptRemarkEnabled(""));
ASSERT_TRUE(engine().isAnalysisOptRemarkEnabled(""));
}
- remark::detail::RemarkEngine &engine() { return *context.getRemarkEngine(); }
+ bool isFinal() const { return GetParam(); }
+
+ remark::detail::RemarkEngine &engine() { return *remarkEngine; }
- remark::RemarkId emit(Location loc, remark::RemarkOpts opts,
- bool analysis = false) {
- auto r = analysis ? remark::analysis(loc, opts) : remark::passed(loc, opts);
+ remark::RemarkId emit(Location loc, remark::RemarkOpts opts) {
+ auto r = remark::passed(loc, opts);
+ return r.getId();
+ }
+
+ remark::RemarkId emitAnalysis(Location loc, remark::RemarkOpts opts) {
+ auto r = remark::analysis(loc, opts);
return r.getId();
}
@@ -861,6 +884,11 @@ class RemarkHistoryTest : public TestWithParam<bool> {
return r.getId();
}
+ remark::RemarkId emitFailed(Location loc, remark::RemarkOpts opts) {
+ auto r = remark::failed(loc, opts);
+ return r.getId();
+ }
+
void flush() { engine().getRemarkEmittingPolicy()->finalize(); }
void expectLinks(remark::RemarkId id, ArrayRef<remark::RemarkId> expected) {
@@ -877,6 +905,8 @@ class RemarkHistoryTest : public TestWithParam<bool> {
std::vector<Record> records;
bool queryInCallback = false;
std::vector<SmallVector<remark::RemarkId>> callbackHistory;
+ std::function<void(const remark::detail::Remark &)> onRecord;
+ remark::detail::RemarkEngine *remarkEngine = nullptr;
// Keep callback state alive while the context drains its remark engine.
MLIRContext context;
};
@@ -944,7 +974,7 @@ TEST_P(RemarkHistoryTest, ExactFieldsAndConjunction) {
.function("kernel");
auto target = emit(loc, opts);
auto otherSub = emit(loc, opts.subCategory("Unroll"));
- auto analysis = emit(loc, opts, /*analysis=*/true);
+ auto analysis = emitAnalysis(loc, opts);
auto other = emit(otherLoc, remark::RemarkOpts::name("Other")
.category("OptOther")
.subCategory("Other")
@@ -993,6 +1023,11 @@ TEST_P(RemarkHistoryTest, ExactFieldsAndConjunction) {
ElementsAre(analysis));
EXPECT_THAT(engine().findRemarks(opts.relatedTo(other)),
ElementsAre(target, analysis));
+ EXPECT_THAT(engine().findRemarks(remark::RemarkOpts{},
+ remark::RemarkKind::RemarkAnalysis),
+ ElementsAre(analysis));
+ EXPECT_THAT(engine().findRemarks(any.kind(remark::RemarkKind::RemarkUnknown)),
+ IsEmpty());
}
TEST_P(RemarkHistoryTest, OwnsQueryAndHistoryStrings) {
@@ -1023,8 +1058,8 @@ TEST_P(RemarkHistoryTest, OwnsQueryAndHistoryStrings) {
EXPECT_THAT(engine().findRemarks(changed), IsEmpty());
EXPECT_THAT(engine().findRemarks(original), ElementsAre(target));
- // Saved queries and copied IDs remain usable after the interned pool resets.
- auto savedIds = engine().findRemarks(query);
+ // Saved queries remain usable after the interned pool resets; IDs are not
+ // reset.
for (bool disable : {false, true}) {
if (disable) {
engine().enableHistory(false);
@@ -1040,7 +1075,6 @@ TEST_P(RemarkHistoryTest, OwnsQueryAndHistoryStrings) {
flush();
EXPECT_THAT(engine().findRemarks(original), ElementsAre(next));
EXPECT_GT(next.getValue(), target.getValue());
- EXPECT_THAT(savedIds, ElementsAre(target));
}
}
@@ -1049,9 +1083,9 @@ TEST_P(RemarkHistoryTest, QueryLinksSnapshotAndSerialization) {
Location loc = UnknownLoc::get(&context);
auto query = remark::RemarkQuery{}.kind(remark::RemarkKind::RemarkAnalysis);
auto first =
- emit(loc, remark::RemarkOpts::name("First").category("Opt"), true);
+ emitAnalysis(loc, remark::RemarkOpts::name("First").category("Opt"));
auto second =
- emit(loc, remark::RemarkOpts::name("Second").category("Opt"), true);
+ emitAnalysis(loc, remark::RemarkOpts::name("Second").category("Opt"));
flush();
remark::RemarkId linked, later, pendingId, noMatches, copiedWrapper;
{
@@ -1065,7 +1099,7 @@ TEST_P(RemarkHistoryTest, QueryLinksSnapshotAndSerialization) {
r << remark::relatedTo(query);
// This report occurs before Linked is reported, but after its query.
later =
- emit(loc, remark::RemarkOpts::name("Later").category("Opt"), true);
+ emitAnalysis(loc, remark::RemarkOpts::name("Later").category("Opt"));
}
}
flush();
@@ -1084,22 +1118,9 @@ TEST_P(RemarkHistoryTest, QueryLinksSnapshotAndSerialization) {
}
flush();
- for (const auto &record : records) {
- if (record.id == linked) {
- EXPECT_THAT(record.related, ElementsAre(first, second));
- EXPECT_THAT(record.serializedLinks,
- ElementsAre(std::to_string(first.getValue()),
- std::to_string(second.getValue())));
- } else if (record.id == copiedWrapper) {
- EXPECT_THAT(record.related, ElementsAre(first, second, pendingId, later));
- } else if (record.id == noMatches) {
- EXPECT_THAT(record.related, IsEmpty());
- EXPECT_THAT(record.serializedLinks, IsEmpty());
- }
- }
- EXPECT_THAT(records, Contains(Field(&Record::id, linked)));
- EXPECT_THAT(records, Contains(Field(&Record::id, copiedWrapper)));
- EXPECT_THAT(records, Contains(Field(&Record::id, noMatches)));
+ expectLinks(linked, {first, second});
+ expectLinks(copiedWrapper, {first, second, pendingId, later});
+ expectLinks(noMatches, {});
}
TEST_P(RemarkHistoryTest, QueryLinksDoNotEnableHistory) {
@@ -1133,7 +1154,7 @@ TEST_P(RemarkHistoryTest, HistoryControlsPreserveFinalPendingRemarks) {
emit(loc, remark::RemarkOpts::name("BeforeEnable").category("Opt"));
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
engine().enableHistory();
- if (GetParam())
+ if (isFinal())
EXPECT_THAT(engine().findRemarks(query), ElementsAre(beforeEnable));
else
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
@@ -1142,7 +1163,7 @@ TEST_P(RemarkHistoryTest, HistoryControlsPreserveFinalPendingRemarks) {
auto beforeClear =
emit(loc, remark::RemarkOpts::name("BeforeClear").category("Opt"));
engine().clearHistory();
- if (GetParam())
+ if (isFinal())
EXPECT_THAT(engine().findRemarks(query), ElementsAre(beforeClear));
else
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
@@ -1153,7 +1174,7 @@ TEST_P(RemarkHistoryTest, HistoryControlsPreserveFinalPendingRemarks) {
engine().enableHistory(false);
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
engine().enableHistory();
- if (GetParam())
+ if (isFinal())
EXPECT_THAT(engine().findRemarks(query), ElementsAre(beforeDisable));
else
EXPECT_THAT(engine().findRemarks(query), IsEmpty());
@@ -1170,7 +1191,7 @@ TEST_P(RemarkHistoryTest, SameKeyQueriesLinkPendingAndEmittedTargets) {
auto query = remark::RemarkQuery::named("Candidate").category("Opt");
auto first = emit(loc, opts);
auto second = emit(loc, opts);
- if (GetParam())
+ if (isFinal())
EXPECT_THAT(engine().findRemarks(query), ElementsAre(second));
else
EXPECT_THAT(engine().findRemarks(query), ElementsAre(first, second));
@@ -1183,7 +1204,7 @@ TEST_P(RemarkHistoryTest, SameKeyQueriesLinkPendingAndEmittedTargets) {
r << remark::relatedTo(query);
}
flush();
- if (GetParam()) {
+ if (isFinal()) {
EXPECT_THAT(engine().findRemarks(query), ElementsAre(second));
EXPECT_THAT(records, Not(Contains(Field(&Record::id, first))));
expectLinks(beforeFlush, {second});
@@ -1200,7 +1221,7 @@ TEST_P(RemarkHistoryTest, SameKeyQueriesLinkPendingAndEmittedTargets) {
r << remark::relatedTo(query);
}
flush();
- if (GetParam())
+ if (isFinal())
expectLinks(sameKey, {second});
else
expectLinks(sameKey, {first, second});
@@ -1216,7 +1237,7 @@ TEST_P(RemarkHistoryTest, SameKeyQueriesLinkPendingAndEmittedTargets) {
emit(loc, opts);
}
flush();
- if (GetParam()) {
+ if (isFinal()) {
expectLinks(consumer, {second, sameKey});
EXPECT_THAT(records, Not(Contains(Field(&Record::id, replaced))));
} else {
@@ -1245,7 +1266,7 @@ TEST_P(RemarkHistoryTest, QueryBeforeReplacementPrunesDiscardedTarget) {
}
auto r2 = emitMissed(loc, opts);
flush();
- if (GetParam()) {
+ if (isFinal()) {
EXPECT_THAT(records,
ElementsAre(Field(&Record::id, r3), Field(&Record::id, r2)));
expectLinks(r3, {});
@@ -1263,7 +1284,7 @@ TEST_P(RemarkHistoryTest, QueryAfterReplacementLinksSurvivingTarget) {
auto r1 = emitMissed(loc, opts);
auto r2 = emitMissed(loc, opts);
auto query = remark::RemarkQuery::named("Candidate").at(loc);
- if (GetParam())
+ if (isFinal())
EXPECT_THAT(engine().findRemarks(query), ElementsAre(r2));
else
EXPECT_THAT(engine().findRemarks(query), ElementsAre(r1, r2));
@@ -1275,7 +1296,7 @@ TEST_P(RemarkHistoryTest, QueryAfterReplacementLinksSurvivingTarget) {
r << remark::relatedTo(query);
}
flush();
- if (GetParam()) {
+ if (isFinal()) {
EXPECT_THAT(records,
ElementsAre(Field(&Record::id, r3), Field(&Record::id, r2)));
expectLinks(r3, {r2});
@@ -1296,8 +1317,10 @@ TEST_P(RemarkHistoryTest, QueryLinksBeforeFinalization) {
consumer = r.getId();
r << remark::relatedTo(remark::RemarkQuery::named("Candidate"));
}
- if (GetParam())
+ if (isFinal())
EXPECT_THAT(records, IsEmpty());
+ else
+ EXPECT_EQ(records.size(), 2u);
flush();
expectLinks(consumer, {target});
EXPECT_EQ(records.size(), 2u);
@@ -1333,6 +1356,7 @@ TEST_P(RemarkHistoryTest, ExplicitLinksPruneDiscardedAndUnknownTargets) {
auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
auto discarded = emit(loc, opts);
auto survivor = emit(loc, opts);
+ // An ID the engine never issued.
remark::RemarkId unknown(1000000);
remark::detail::OptRemarkPass consumer(
loc, remark::RemarkOpts::name("Consumer").category("Opt"));
@@ -1343,7 +1367,7 @@ TEST_P(RemarkHistoryTest, ExplicitLinksPruneDiscardedAndUnknownTargets) {
consumer.addRelatedRemark(unknown);
engine().report(std::move(consumer));
flush();
- if (GetParam()) {
+ if (isFinal()) {
expectLinks(consumerId, {survivor});
EXPECT_EQ(records.size(), 2u);
EXPECT_THAT(records, Not(Contains(Field(&Record::id, discarded))));
@@ -1352,6 +1376,9 @@ TEST_P(RemarkHistoryTest, ExplicitLinksPruneDiscardedAndUnknownTargets) {
}
}
+// Final prunes a link whose target is still in flight when the batch is
+// finalized, even though the target is emitted by the next batch. This is a
+// deliberate loss: links are never resolved against future batches.
TEST_P(RemarkHistoryTest, ExplicitLinksDoNotWaitForFutureBatches) {
Location loc = UnknownLoc::get(&context);
remark::RemarkId consumer, futureId;
@@ -1363,7 +1390,7 @@ TEST_P(RemarkHistoryTest, ExplicitLinksDoNotWaitForFutureBatches) {
loc,
remark::RemarkOpts::name("Consumer").category("Opt").relatedTo(future));
flush();
- if (GetParam())
+ if (isFinal())
expectLinks(consumer, {});
else
expectLinks(consumer, {futureId});
@@ -1371,7 +1398,7 @@ TEST_P(RemarkHistoryTest, ExplicitLinksDoNotWaitForFutureBatches) {
flush();
EXPECT_EQ(records.size(), 2u);
EXPECT_THAT(records, Contains(Field(&Record::id, futureId)));
- if (GetParam())
+ if (isFinal())
expectLinks(consumer, {});
}
@@ -1406,7 +1433,7 @@ TEST_P(RemarkHistoryTest, TransitiveLinksAndCyclesEmitEachSurvivorOnce) {
expectLinks(cycleA, {cycleB});
expectLinks(cycleB, {cycleA});
expectLinks(self, {self});
- if (GetParam())
+ if (isFinal())
EXPECT_THAT(
records,
ElementsAre(Field(&Record::id, root), Field(&Record::id, middle),
@@ -1420,14 +1447,13 @@ TEST_P(RemarkHistoryTest, SharedTargetsEmitOnceAndQueriesUseCreationOrder) {
engine().enableHistory();
Location loc = UnknownLoc::get(&context);
auto child =
- emit(loc, remark::RemarkOpts::name("Child").category("Opt"), true);
+ emitAnalysis(loc, remark::RemarkOpts::name("Child").category("Opt"));
auto firstParent = emit(
loc,
remark::RemarkOpts::name("FirstParent").category("Opt").relatedTo(child));
- auto secondParent = emit(
- loc,
- remark::RemarkOpts::name("SecondParent").category("Opt").relatedTo(child),
- true);
+ auto secondParent = emitAnalysis(loc, remark::RemarkOpts::name("SecondParent")
+ .category("Opt")
+ .relatedTo(child));
flush();
auto query = remark::RemarkQuery{};
EXPECT_THAT(engine().findRemarks(query),
@@ -1460,7 +1486,7 @@ TEST_P(RemarkHistoryTest, StreamerCanQueryCurrentReportAndActiveDrain) {
auto first = emit(loc, remark::RemarkOpts::name("First").category("Opt"));
auto second = emit(loc, remark::RemarkOpts::name("Second").category("Opt"));
flush();
- if (GetParam())
+ if (isFinal())
EXPECT_THAT(callbackHistory, ElementsAre(ElementsAre(first, second),
ElementsAre(first, second)));
else
@@ -1468,6 +1494,182 @@ TEST_P(RemarkHistoryTest, StreamerCanQueryCurrentReportAndActiveDrain) {
ElementsAre(ElementsAre(first), ElementsAre(first, second)));
}
+// The consumer shares the Final identity of the pending target it queries, so
+// its own report replaces that target and the link is pruned; the link is not
+// redirected to the consumer.
+TEST_P(RemarkHistoryTest, ConsumerReplacingItsQueryTargetLosesTheLink) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
+ auto target = emitMissed(loc, opts);
+ remark::RemarkId consumer;
+ {
+ auto r = remark::missed(loc, opts);
+ consumer = r.getId();
+ r << remark::relatedTo(remark::RemarkQuery::named("Candidate").at(loc));
+ }
+ flush();
+ if (isFinal()) {
+ EXPECT_THAT(records, ElementsAre(Field(&Record::id, consumer)));
+ expectLinks(consumer, {});
+ EXPECT_THAT(engine().findRemarks(remark::RemarkQuery::named("Candidate")),
+ ElementsAre(consumer));
+ } else {
+ EXPECT_EQ(records.size(), 2u);
+ expectLinks(consumer, {target});
+ }
+}
+
+// function() narrows a query, but the function is not part of the Final
+// identity: a same-identity report from another function replaces the matched
+// target and the link is pruned.
+TEST_P(RemarkHistoryTest, FunctionScopedQueryTargetReplacedByOtherFunction) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
+ auto kernelTarget = emit(loc, opts.function("kernel"));
+ remark::RemarkId consumer;
+ {
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("Consumer").category("Opt"));
+ consumer = r.getId();
+ r << remark::relatedTo(
+ remark::RemarkQuery::named("Candidate").function("kernel"));
+ }
+ auto helperTarget = emit(loc, opts.function("helper"));
+ flush();
+ if (isFinal()) {
+ EXPECT_THAT(records, Not(Contains(Field(&Record::id, kernelTarget))));
+ EXPECT_THAT(records, Contains(Field(&Record::id, helperTarget)));
+ expectLinks(consumer, {});
+ } else {
+ expectLinks(consumer, {kernelTarget});
+ }
+}
+
+// An explicit link and query matches for the same target produce one link.
+TEST_P(RemarkHistoryTest, ExplicitAndQueryLinksToSameTargetAreLinkedOnce) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto target =
+ emitAnalysis(loc, remark::RemarkOpts::name("Target").category("Opt"));
+ auto query = remark::RemarkQuery::named("Target");
+ remark::RemarkId consumer;
+ {
+ auto r = remark::passed(
+ loc,
+ remark::RemarkOpts::name("Consumer").category("Opt").relatedTo(target));
+ consumer = r.getId();
+ r << remark::relatedTo(query) << remark::relatedTo(query);
+ }
+ flush();
+ expectLinks(consumer, {target});
+}
+
+// A Failure and a Passed remark at the same site have different Final
+// identities: both are searchable, both can be linked, and both are emitted.
+TEST_P(RemarkHistoryTest, FailedAndPassedAtOneSiteAreDistinctIdentities) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
+ auto failed = emitFailed(loc, opts);
+ auto passed = emit(loc, opts);
+ auto query = remark::RemarkQuery::named("Candidate").at(loc);
+ EXPECT_THAT(engine().findRemarks(query), ElementsAre(failed, passed));
+ EXPECT_THAT(
+ engine().findRemarks(query.kind(remark::RemarkKind::RemarkFailure)),
+ ElementsAre(failed));
+ remark::RemarkId consumer;
+ {
+ auto r = remark::passed(
+ loc, remark::RemarkOpts::name("Consumer").category("Opt"));
+ consumer = r.getId();
+ r << remark::relatedTo(query);
+ }
+ flush();
+ expectLinks(consumer, {failed, passed});
+ EXPECT_EQ(records.size(), 3u);
+}
+
+// A streamer callback may report remarks while Final drains a batch. The new
+// remark joins the next batch and does not replace the remark being delivered,
+// even with the same identity. Under All it is delivered immediately.
+TEST_P(RemarkHistoryTest, CallbackReportsDuringDrainJoinNextBatch) {
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ auto opts = remark::RemarkOpts::name("Candidate").category("Opt");
+ remark::RemarkId nested;
+ bool reported = false;
+ onRecord = [&](const remark::detail::Remark &) {
+ // Under All the nested report is delivered, and this callback runs again,
+ // before `nested` is assigned; guard with a flag set first.
+ if (reported)
+ return;
+ reported = true;
+ nested = emit(loc, opts);
+ };
+ auto first = emit(loc, opts);
+ flush();
+ EXPECT_THAT(records, Contains(Field(&Record::id, first)));
+ EXPECT_THAT(engine().findRemarks(remark::RemarkQuery::named("Candidate")),
+ ElementsAre(first, nested));
+ if (isFinal())
+ EXPECT_EQ(records.size(), 1u);
+ flush();
+ EXPECT_THAT(records, ElementsAre(Field(&Record::id, first),
+ Field(&Record::id, nested)));
+}
+
+// A streamer callback that queries during the drain run by the engine
+// destructor must not crash: the context's engine pointer is already null
+// there, so the fixture reaches the engine through a retained pointer. Nothing
+// can be asserted after teardown; this test guards against a crash.
+TEST_P(RemarkHistoryTest, StreamerQueriesWhileContextDestroysEngine) {
+ engine().enableHistory();
+ queryInCallback = true;
+ Location loc = UnknownLoc::get(&context);
+ emit(loc, remark::RemarkOpts::name("Unflushed").category("Opt"));
+ // No flush(): under Final the engine destructor drains this remark.
+}
+
+#if LLVM_ENABLE_THREADS
+// Concurrent reports, query-based links, and queries with history enabled.
+// All delivers remarks on the reporting threads and the fixture records are
+// not synchronized, so only Final is exercised.
+TEST_P(RemarkHistoryTest, ConcurrentReportsAndQueries) {
+ if (!isFinal())
+ GTEST_SKIP() << "All delivers remarks on the reporting threads";
+ engine().enableHistory();
+ Location loc = UnknownLoc::get(&context);
+ constexpr unsigned numThreads = 4, perThread = 50;
+ std::vector<std::thread> threads;
+ for (unsigned t = 0; t < numThreads; ++t)
+ threads.emplace_back([&, t] {
+ std::string function = "f" + std::to_string(t);
+ for (unsigned i = 0; i < perThread; ++i) {
+ // Distinct names: Final identity ignores the function, so same-named
+ // remarks at one location would replace each other across threads.
+ std::string name = function + "_" + std::to_string(i);
+ remark::passed(
+ loc,
+ remark::RemarkOpts::name(name).category("Opt").function(function))
+ << remark::relatedTo(remark::RemarkQuery{}.function(function));
+ (void)engine().findRemarks(remark::RemarkQuery{});
+ }
+ });
+ for (std::thread &thread : threads)
+ thread.join();
+ flush();
+ ASSERT_EQ(records.size(), size_t{numThreads * perThread});
+ llvm::DenseSet<uint64_t> ids;
+ for (const Record &record : records)
+ ids.insert(record.id.getValue());
+ for (const Record &record : records)
+ for (remark::RemarkId related : record.related)
+ EXPECT_TRUE(ids.contains(related.getValue()));
+}
+#endif
+
INSTANTIATE_TEST_SUITE_P(EmittingPolicies, RemarkHistoryTest, Bool(),
[](const TestParamInfo<bool> &info) {
return info.param ? "Final" : "All";
More information about the Mlir-commits
mailing list