[Mlir-commits] [mlir] [MLIR] Emit final-policy remarks in deterministic order (PR #224616)
Henrich Lauko
llvmlistbot at llvm.org
Mon Sep 21 06:41:15 PDT 2026
https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/224616
>From 5b77f2f8cee5519123ef711be156656e466babec Mon Sep 17 00:00:00 2001
From: Henrich Lauko <hlauko at nvidia.com>
Date: Fri, 18 Sep 2026 13:09:19 +0000
Subject: [PATCH] [MLIR][Remark] Emit final-policy remarks in deterministic
order
RemarkEmittingPolicyFinal stores remarks in a DenseSet whose hash
includes 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 Identity: 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.
Identity is a private nested type of the policy, since nothing outside
decides what that policy treats as the same remark. Its asTuple() is the
only place the field list appears and drives both hashing and equality,
so the two cannot disagree about which fields matter. IdentityInfo is
passed as the KeyInfoT of the MapVector's DenseMap rather than
specializing llvm::DenseMapInfo, which keeps this inside mlir::remark
and removes DenseMapInfo<Remark>.
Behaviour change: order only. The fields that make up the identity are
unchanged.
remark-final.mlir switches to ordered CHECK lines. New unit tests cover
first-position-last-content replacement, and two identity
normalizations that had no coverage: an empty name equals the
"<unknown remark name>" placeholder, and category plus sub-category
equals the combined "category:sub" name.
Assisted-by: Claude Code (Claude Fable 5.1)
---
mlir/docs/Remarks.md | 26 +++++++-----
mlir/include/mlir/IR/Remarks.h | 72 +++++++++++++++++++-------------
mlir/lib/IR/Remarks.cpp | 10 ++---
mlir/test/Pass/remark-final.mlir | 34 ++++++++-------
mlir/unittests/IR/RemarkTest.cpp | 62 +++++++++++++++++++++++++--
5 files changed, 143 insertions(+), 61 deletions(-)
diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md
index 3f468e730003d..9f62a1cefbae5 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 the location, remark name, combined category name and kind.
+Arguments are never 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 9e406f2263f1e..d2c7f652a3845 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"
@@ -665,19 +666,56 @@ 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 identity.
+/// 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;
+ /// Location, remark name, combined category name and 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 Identity {
+ Location loc;
+ std::string remarkName;
+ std::string combinedCategoryName;
+ RemarkKind kind;
+
+ explicit Identity(const detail::Remark &remark)
+ : loc(remark.getLocation()), remarkName(remark.getRemarkName()),
+ combinedCategoryName(remark.getCombinedCategoryName()),
+ kind(remark.getRemarkKind()) {}
+
+ auto asTuple() const {
+ return std::tie(loc, remarkName, combinedCategoryName, kind);
+ }
+ };
+
+ struct IdentityInfo {
+ static unsigned getHashValue(const Identity &identity) {
+ return std::apply(
+ [](const auto &...fields) {
+ return static_cast<unsigned>(llvm::hash_combine(fields...));
+ },
+ identity.asTuple());
+ }
+
+ static bool isEqual(const Identity &lhs, const Identity &rhs) {
+ return lhs.asTuple() == rhs.asTuple();
+ }
+ };
+
+ /// Remarks reported since the last finalize(), keyed by identity and kept
+ /// in first-report order.
+ llvm::MapVector<Identity, detail::Remark,
+ llvm::DenseMap<Identity, unsigned, IdentityInfo>>
+ postponedRemarks;
public:
RemarkEmittingPolicyFinal();
void reportRemark(const detail::Remark &remark) override {
- postponedRemarks.erase(remark);
- postponedRemarks.insert(remark);
+ postponedRemarks.insert_or_assign(Identity(remark), remark);
}
/// Emits and drains all stored remarks. Related remarks are printed right
@@ -761,26 +799,4 @@ LogicalResult enableOptimizationRemarks(
} // namespace mlir::remark
-/// Two remarks are the same for RemarkEmittingPolicyFinal when they have the
-/// same location, remark name, combined category name and kind.
-namespace llvm {
-template <>
-struct DenseMapInfo<mlir::remark::detail::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) {
- 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..ef49af62b9b5d 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,8 +388,8 @@ 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) {
- if (remark.getId() && childIds.count(remark.getId().getValue()))
+ for (const detail::Remark &remark : llvm::make_second_range(remarks)) {
+ if (remark.getId() && childIds.contains(remark.getId().getValue()))
continue; // will be printed grouped under its parent
reportImpl(remark);
diff --git a/mlir/test/Pass/remark-final.mlir b/mlir/test/Pass/remark-final.mlir
index acf768e6e51f8..47c132a682aa2 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..002c62d13c45b 100644
--- a/mlir/unittests/IR/RemarkTest.cpp
+++ b/mlir/unittests/IR/RemarkTest.cpp
@@ -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,44 @@ 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);
+
+ // An empty name and the literal placeholder are the same identity.
+ remark::passed(loc, remark::RemarkOpts::name("").category("LoopUnroll"))
+ << "unnamed 1";
+ remark::passed(loc, remark::RemarkOpts::name("<unknown remark name>")
+ .category("LoopUnroll"))
+ << "unnamed 2";
+
+ // Category plus sub-category and the combined "category:sub" form are the
+ // same identity.
+ remark::passed(loc, remark::RemarkOpts::name("Vec")
+ .category("LoopVectorize")
+ .subCategory("inner"))
+ << "combined 1";
+ remark::passed(
+ loc, remark::RemarkOpts::name("Vec").category("LoopVectorize: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 Mlir-commits
mailing list