[llvm-branch-commits] [mlir] [MLIR][Remark] Emit final-policy remarks in deterministic order (PR #224616)
Henrich Lauko via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Fri Sep 18 05:20:28 PDT 2026
https://github.com/xlauko created https://github.com/llvm/llvm-project/pull/224616
Stacked on #224607.
`RemarkEmittingPolicyFinal` stores remarks in a `DenseSet` keyed on the location pointer, so output order depends on heap layout and changes between runs. Twenty runs of `mlir/test/Pass/remark-final.mlir` gave fourteen different orders, which is why the test uses `CHECK-DAG`. Downstream tests had to pin `--remark-policy=all` for the same reason.
This change stores remarks in a `MapVector` keyed by a new `RemarkIdentity`: location, remark name, combined category name and kind, the same fields the `DenseSet` compared. A repeated identity overwrites the stored remark in place, so a remark is printed where its identity was first reported, with the content it last had. Root remarks come out in first-report order; linked children still follow their parent. `DenseMapInfo<Remark>` is removed, and `RemarkIdentity` is now the one place that says what the final policy treats as the same remark.
Why first-report position rather than last: `MapVector::insert_or_assign` keeps the existing slot in O(1). Erase-then-insert would need `MapVector::erase`, which re-indexes every entry, so a fail-retry-succeed loop goes quadratic. First-report order is also where `--remark-policy=all` shows a key's first appearance. If last-report order is preferred, a sequence number per entry and a sort in `finalize()` gives it at O(n log n) once.
Behaviour change: order only. The identity is unchanged. Whether the identity is right is a separate question I will raise in a follow-up: the docs said a later `passed` replaces an earlier `failed` at the same location, but `kind` has been part of the key since #180953, so both are shown today. This PR keeps the docs neutral on that.
Tests:
- `remark-final.mlir` switches from `CHECK-DAG` to ordered `CHECK` lines, with the count pinned by `--implicit-check-not` from #224607.
- `TestRemarkFinalOrder`: A, B, A' where A' shares A's identity but has a different function name; output is A' then B.
- `TestRemarkFinalIdentityFields`: two unnamed remarks merge under the placeholder name; category plus sub-category compare as the combined form, and a different sub-category stays separate.
Assisted-by: Claude Code (Claude Fable 5.1). I read and reviewed the change before opening it.
>From 44ed10dbad4e1758c1b5143982301b1c8e18fd7e Mon Sep 17 00:00:00 2001
From: Henrich Lauko <hlauko at nvidia.com>
Date: Fri, 18 Sep 2026 12:20:16 +0000
Subject: [PATCH] [MLIR][Remark] Emit final-policy remarks in deterministic
order
RemarkEmittingPolicyFinal stores remarks in a DenseSet keyed on the
location pointer, so the output order depends on heap layout and changes
between runs. Twenty runs of mlir/test/Pass/remark-final.mlir gave
fourteen different orders, which is why the test uses CHECK-DAG.
Store remarks in a MapVector keyed by a new RemarkIdentity: location,
remark name, combined category name and kind, the same fields the
DenseSet compared. A repeated identity overwrites the stored remark in
place, so a remark is printed where its identity was first reported with
the content it last had. Root remarks come out in first-report order and
linked children still follow their parent. DenseMapInfo<Remark> is
removed; RemarkIdentity is now the one place that says what the final
policy treats as the same remark.
Behaviour change: order only. The identity is unchanged.
remark-final.mlir switches to ordered CHECK lines. New unit tests cover
first-position-last-content replacement, and the identity fields that
had no coverage: the unnamed-remark placeholder and the combined
category name.
Assisted-by: Claude Code (Claude Fable 5.1)
---
mlir/docs/Remarks.md | 26 +++++----
mlir/include/mlir/IR/Remarks.h | 94 +++++++++++++++++---------------
mlir/lib/IR/Remarks.cpp | 8 +--
mlir/test/Pass/remark-final.mlir | 34 +++++++-----
mlir/unittests/IR/RemarkTest.cpp | 67 ++++++++++++++++++++---
5 files changed, 151 insertions(+), 78 deletions(-)
diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md
index 3f468e730003d..3d6190dac23de 100644
--- a/mlir/docs/Remarks.md
+++ b/mlir/docs/Remarks.md
@@ -202,21 +202,27 @@ 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. Calling it again emits only remarks reported since.
-
-**Example:** Only the successful remark is emitted:
+Stores remarks until `finalize()` is called and emits only the **last** remark
+reported for each identity. This is useful in multi-pass compilers where several
+passes report on the same thing and only the final report should be shown. The
+identity is currently the location, remark name, combined category name and
+remark kind; arguments are not part of it. Root remarks are emitted in the order
+in which their identity was first reported, with linked remarks right after the
+remark that references them, so the output does not depend on hash order.
+`finalize()` drains the stored remarks. Calling it again emits only remarks
+reported since.
+
+**Example:** Only the second remark is emitted, because both share an
+identity.
```c++
auto opts = remark::RemarkOpts::name("Unroller").category("LoopUnroll");
-// First pass: reports failure
-remark::failed(loc, opts) << "Loop could not be unrolled";
+// First attempt.
+remark::passed(loc, opts) << "Loop unrolled by 2";
-// Later pass: reports success (this is the one emitted)
-remark::passed(loc, opts) << "Loop unrolled successfully";
+// A later pass revisits the same loop. This is the one emitted.
+remark::passed(loc, opts) << "Loop unrolled by 4";
```
You can also implement custom policies by inheriting from the policy interface.
diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index f0cdedbadb7e7..5aae8961089a2 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/MapVector.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/Remarks/Remark.h"
@@ -356,6 +357,48 @@ inline Remark &operator<<(Remark &r, const Remark::Arg &kv) {
return r;
}
+//===----------------------------------------------------------------------===//
+// RemarkIdentity
+//===----------------------------------------------------------------------===//
+
+/// The fields RemarkEmittingPolicyFinal compares to decide that two remarks
+/// describe the same thing: location, remark name, combined category name and
+/// remark kind. Arguments, function name and remark ID are not part of the
+/// identity, so a later remark with the same identity replaces an earlier one.
+struct RemarkIdentity {
+ Location loc;
+ std::string remarkName;
+ std::string combinedCategoryName;
+ RemarkKind kind;
+
+ explicit RemarkIdentity(const Remark &remark)
+ : loc(remark.getLocation()), remarkName(remark.getRemarkName()),
+ combinedCategoryName(remark.getCombinedCategoryName()),
+ kind(remark.getRemarkKind()) {}
+};
+
+} // namespace mlir::remark::detail
+
+namespace llvm {
+template <>
+struct DenseMapInfo<mlir::remark::detail::RemarkIdentity> {
+ using RemarkIdentity = mlir::remark::detail::RemarkIdentity;
+
+ static unsigned getHashValue(const RemarkIdentity &identity) {
+ return llvm::hash_combine(identity.loc, identity.remarkName,
+ identity.combinedCategoryName, identity.kind);
+ }
+
+ static bool isEqual(const RemarkIdentity &lhs, const RemarkIdentity &rhs) {
+ return lhs.loc == rhs.loc && lhs.kind == rhs.kind &&
+ lhs.remarkName == rhs.remarkName &&
+ lhs.combinedCategoryName == rhs.combinedCategoryName;
+ }
+};
+} // namespace llvm
+
+namespace mlir::remark::detail {
+
//===----------------------------------------------------------------------===//
// Shorthand aliases for different kinds of remarks.
//===----------------------------------------------------------------------===//
@@ -665,19 +708,21 @@ class RemarkEmittingPolicyAll : public detail::RemarkEmittingPolicyBase {
void finalize() override {}
};
-/// Policy that emits only the last remark reported for each identity, see
-/// DenseMapInfo<Remark>. Remarks are stored until finalize().
+/// Policy that emits only the last remark reported for each RemarkIdentity.
+/// Remarks are stored until finalize(). A later remark with the same identity
+/// replaces the stored one in place, so root remarks are emitted in the order
+/// in which their identity was first reported.
class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
private:
- /// Remarks reported since the last finalize().
- llvm::DenseSet<detail::Remark> postponedRemarks;
+ /// Remarks reported since the last finalize(), keyed by identity and kept
+ /// in first-report order.
+ llvm::MapVector<detail::RemarkIdentity, detail::Remark> postponedRemarks;
public:
RemarkEmittingPolicyFinal();
void reportRemark(const detail::Remark &remark) override {
- postponedRemarks.erase(remark);
- postponedRemarks.insert(remark);
+ postponedRemarks.insert_or_assign(detail::RemarkIdentity(remark), remark);
}
/// Emits and drains all stored remarks. Related remarks are printed right
@@ -761,41 +806,4 @@ LogicalResult enableOptimizationRemarks(
} // namespace mlir::remark
-// DenseMapInfo specialization for Remark
-namespace llvm {
-template <>
-struct DenseMapInfo<mlir::remark::detail::Remark> {
- static constexpr StringRef kEmptyKey = "<EMPTY_KEY>";
-
- /// Helper to provide a static dummy context for sentinel keys.
- static mlir::MLIRContext *getStaticDummyContext() {
- static mlir::MLIRContext dummyContext;
- return &dummyContext;
- }
-
- /// Create an empty remark
- /// Compute the hash value of the remark
- static unsigned getHashValue(const mlir::remark::detail::Remark &remark) {
- return llvm::hash_combine(
- remark.getLocation().getAsOpaquePointer(),
- llvm::hash_value(remark.getRemarkName()),
- llvm::hash_value(remark.getCombinedCategoryName()),
- static_cast<unsigned>(remark.getRemarkKind()));
- }
-
- static bool isEqual(const mlir::remark::detail::Remark &lhs,
- const mlir::remark::detail::Remark &rhs) {
- // Check for empty keys first.
- if (lhs.getRemarkName() == kEmptyKey || rhs.getRemarkName() == kEmptyKey) {
- return lhs.getRemarkName() == rhs.getRemarkName();
- }
-
- // For regular remarks, compare key identifying fields
- return lhs.getLocation() == rhs.getLocation() &&
- lhs.getRemarkName() == rhs.getRemarkName() &&
- lhs.getCombinedCategoryName() == rhs.getCombinedCategoryName() &&
- lhs.getRemarkKind() == rhs.getRemarkKind();
- }
-};
-} // namespace llvm
#endif // MLIR_IR_REMARKS_H
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index 57e43b8a0d090..4b295827f11e7 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -12,6 +12,7 @@
#include "mlir/IR/Diagnostics.h"
#include "mlir/IR/Value.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
@@ -370,14 +371,13 @@ void RemarkEmittingPolicyFinal::finalize() {
// Take the pending remarks so that a second finalize(), e.g. from the engine
// destructor after an explicit call, does not emit them again.
- llvm::DenseSet<detail::Remark> remarks;
- remarks.swap(postponedRemarks);
+ auto remarks = postponedRemarks.takeVector();
// Build ID -> Remark* lookup for resolving related remark references.
llvm::DenseMap<uint64_t, const detail::Remark *> idMap;
llvm::DenseSet<uint64_t> childIds; // IDs referenced as children
- for (const auto &remark : remarks) {
+ for (const detail::Remark &remark : llvm::make_second_range(remarks)) {
if (remark.getId())
idMap[remark.getId().getValue()] = &remark;
for (auto relId : remark.getRelatedRemarkIds())
@@ -388,7 +388,7 @@ void RemarkEmittingPolicyFinal::finalize() {
// Parent remarks are emitted first, followed by their related (child)
// remarks. Child-only remarks are skipped at the top level to avoid
// duplication.
- for (const auto &remark : remarks) {
+ for (const detail::Remark &remark : llvm::make_second_range(remarks)) {
if (remark.getId() && childIds.count(remark.getId().getValue()))
continue; // will be printed grouped under its parent
diff --git a/mlir/test/Pass/remark-final.mlir b/mlir/test/Pass/remark-final.mlir
index acf768e6e51f8..efa7086a2b729 100644
--- a/mlir/test/Pass/remark-final.mlir
+++ b/mlir/test/Pass/remark-final.mlir
@@ -5,19 +5,25 @@ module @foo {
"test.op"() : () -> ()
}
-// 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.
+// The two passed remarks in "category-1-passed" share an identity, so only the
+// second survives, in the first one's position. 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-DAG: remark: [Passed] test-remark | Category:category-1-passed |{{.*}}Remark="This is a test passed remark",
-// CHECK-DAG: remark: [Failure] test-remark | Category:category-2-failed
-// CHECK-DAG: remark: [Analysis] test-remark | Category:category-2-analysis
-// CHECK-DAG: remark: [Passed] test-remark | Category:category-link |{{.*}}RelatedTo=
-// CHECK-DAG: remark: [Analysis] test-remark | Category:category-link
+// 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
+// CHECK: remark: [Analysis] test-remark | Category:category-2-analysis
+// CHECK: remark: [Passed] test-remark | Category:category-link |{{.*}}RelatedTo=
+// CHECK: remark: [Analysis] test-remark | Category:category-link
-// CHECK-YAML-DAG: --- !Passed
-// CHECK-YAML-DAG: --- !Failure
-// CHECK-YAML-DAG: --- !Analysis
-// CHECK-YAML-DAG: --- !Passed
-// CHECK-YAML-DAG: --- !Analysis
+// CHECK-YAML: --- !Passed
+// CHECK-YAML-NEXT: Pass:{{.*}}category-1-passed
+// CHECK-YAML: --- !Failure
+// CHECK-YAML-NEXT: Pass:{{.*}}category-2-failed
+// CHECK-YAML: --- !Analysis
+// CHECK-YAML-NEXT: Pass:{{.*}}category-2-analysis
+// CHECK-YAML: --- !Passed
+// CHECK-YAML-NEXT: Pass:{{.*}}category-link
+// CHECK-YAML: --- !Analysis
+// CHECK-YAML-NEXT: Pass:{{.*}}category-link
diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp
index 325d7f24c8aa4..ac2f24d474a41 100644
--- a/mlir/unittests/IR/RemarkTest.cpp
+++ b/mlir/unittests/IR/RemarkTest.cpp
@@ -428,11 +428,11 @@ class RecordingStreamer : public remark::detail::MLIRRemarkStreamerBase {
static LogicalResult enableFinalPolicy(MLIRContext &context,
std::vector<std::string> &emitted,
- StringRef category) {
+ StringRef passedCategory) {
mlir::remark::RemarkCategories cats{/*all=*/std::nullopt,
- /*passed=*/category.str(),
+ /*passed=*/passedCategory.str(),
/*missed=*/std::nullopt,
- /*analysis=*/category.str(),
+ /*analysis=*/passedCategory.str(),
/*failed=*/std::nullopt};
return remark::enableOptimizationRemarks(
context, std::make_unique<RecordingStreamer>(emitted),
@@ -440,6 +440,25 @@ static LogicalResult enableFinalPolicy(MLIRContext &context,
/*printAsEmitRemarks=*/false);
}
+// The final policy emits remarks in the order their identity was first
+// reported, with the content of the last report for that identity.
+TEST(Remark, TestRemarkFinalOrder) {
+ std::vector<std::string> emitted;
+ {
+ MLIRContext context;
+ ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
+ Location locA = FileLineColLoc::get(&context, "test.cpp", 1, 5);
+ Location locB = FileLineColLoc::get(&context, "test.cpp", 2, 5);
+ auto opts = remark::RemarkOpts::name("Unroller").category("LoopUnroll");
+
+ remark::passed(locA, opts) << "A first";
+ remark::passed(locB, opts) << "B";
+ // The function name is not part of the identity.
+ remark::passed(locA, opts.function("other")) << "A last";
+ }
+ EXPECT_THAT(emitted, ElementsAre("Unroller: A last", "Unroller: B"));
+}
+
// finalize() drains the stored remarks. mlir-opt calls it explicitly and the
// engine destructor calls it again; each call emits only the remarks reported
// since the previous one, and an identity drained by one call can be reported
@@ -457,8 +476,7 @@ TEST(Remark, TestRemarkFinalDrains) {
remark::passed(loc, first) << "first";
remark::passed(loc, second) << "second";
policy->finalize();
- EXPECT_THAT(emitted,
- UnorderedElementsAre("First: first", "Second: second"));
+ EXPECT_THAT(emitted, ElementsAre("First: first", "Second: second"));
// Nothing pending: a repeated call emits nothing.
policy->finalize();
@@ -469,8 +487,43 @@ TEST(Remark, TestRemarkFinalDrains) {
remark::passed(loc, first) << "first again";
EXPECT_EQ(emitted.size(), 2u);
}
- ASSERT_EQ(emitted.size(), 3u);
- EXPECT_EQ(emitted[2], "First: first again");
+ EXPECT_THAT(emitted, ElementsAre("First: first", "Second: second",
+ "First: first again"));
+}
+
+// Identity uses the same accessors as the printed remark: an empty name is the
+// "<unknown remark name>" placeholder, and category plus sub-category compare
+// as their combined "category:sub" form.
+TEST(Remark, TestRemarkFinalIdentityFields) {
+ std::vector<std::string> emitted;
+ {
+ MLIRContext context;
+ ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "Loop.*")));
+ Location loc = FileLineColLoc::get(&context, "test.cpp", 1, 5);
+
+ // Two unnamed remarks share an identity.
+ remark::passed(loc, remark::RemarkOpts::name("").category("LoopUnroll"))
+ << "unnamed 1";
+ remark::passed(loc, remark::RemarkOpts::name("").category("LoopUnroll"))
+ << "unnamed 2";
+
+ // Category and sub-category form one combined name.
+ remark::passed(loc, remark::RemarkOpts::name("Vec")
+ .category("LoopVectorize")
+ .subCategory("inner"))
+ << "combined 1";
+ remark::passed(loc, remark::RemarkOpts::name("Vec")
+ .category("LoopVectorize")
+ .subCategory("inner"))
+ << "combined 2";
+ // A different sub-category is a different identity.
+ remark::passed(loc, remark::RemarkOpts::name("Vec")
+ .category("LoopVectorize")
+ .subCategory("outer"))
+ << "outer";
+ }
+ EXPECT_THAT(emitted, ElementsAre("<unknown remark name>: unnamed 2",
+ "Vec: combined 2", "Vec: outer"));
}
// A RelatedTo link only resolves between remarks drained by the same
More information about the llvm-branch-commits
mailing list