[Mlir-commits] [mlir] [MLIR] Emit final-policy remarks in deterministic order (PR #224616)
Henrich Lauko
llvmlistbot at llvm.org
Wed Sep 30 03:09:22 PDT 2026
https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/224616
>From 2948f4a0569388a24cd8b727ff2a5ad29445d571 Mon Sep 17 00:00:00 2001
From: Henrich Lauko <hlauko at nvidia.com>
Date: Wed, 30 Sep 2026 09:55:27 +0000
Subject: [PATCH] [MLIR][Remark] Emit final-policy remarks in deterministic
order
RemarkEmittingPolicyFinal stores remarks in a DenseSet whose hash covers the
location pointer and the hash seed, so finalize() emitted them in bucket
order, which depends on the build and on where things landed in memory. That
is why mlir/test/Pass/remark-final.mlir used CHECK-DAG.
The engine assigns every remark a RemarkId from a monotonic counter when it is
created, and the set already keeps the newer of two remarks with the same
identity. finalize() now sorts the drained remarks by that ID before emitting,
so remarks come out in creation order and a replaced identity takes the
position of its last report. Linked remarks still follow their parent.
Order only. The identity, DenseMapInfo<Remark> and the header are unchanged.
Only remarks handed to the policy outside the engine have no ID; the unit
tests that do so assert unordered or single results.
Assisted-by: Claude Code (Claude Fable 5.1)
---
mlir/docs/Remarks.md | 3 ++-
mlir/include/mlir/IR/Remarks.h | 3 ++-
mlir/lib/IR/Remarks.cpp | 14 +++++++++----
mlir/test/Pass/remark-final.mlir | 34 +++++++++++++++++++-------------
mlir/unittests/IR/RemarkTest.cpp | 26 ++++++++++++++++++++----
5 files changed, 56 insertions(+), 24 deletions(-)
diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md
index 3f468e730003d..e747f7bc8f595 100644
--- a/mlir/docs/Remarks.md
+++ b/mlir/docs/Remarks.md
@@ -205,7 +205,8 @@ Emits **all** remarks unconditionally.
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.
+remarks and emits them in the order in which they were created. Calling it
+again emits only remarks reported since.
**Example:** Only the successful remark is emitted:
diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 9e406f2263f1e..0ecc63717b5ae 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -666,7 +666,8 @@ class RemarkEmittingPolicyAll : public detail::RemarkEmittingPolicyBase {
};
/// Policy that emits only the last remark reported for each identity, see
-/// DenseMapInfo<Remark>. Remarks are stored until finalize().
+/// DenseMapInfo<Remark>. Remarks are stored until finalize(), which emits them
+/// in creation order, so the output does not depend on hash order.
class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
private:
/// Remarks reported since the last finalize().
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index 57e43b8a0d090..2c6c533bb62ae 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -369,9 +369,15 @@ 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.
- llvm::DenseSet<detail::Remark> remarks;
- remarks.swap(postponedRemarks);
+ // 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 detail::Remark &lhs, const detail::Remark &rhs) {
+ return lhs.getId().getValue() < rhs.getId().getValue();
+ });
// Build ID -> Remark* lookup for resolving related remark references.
llvm::DenseMap<uint64_t, const detail::Remark *> idMap;
@@ -389,7 +395,7 @@ void RemarkEmittingPolicyFinal::finalize() {
// 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()))
+ 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..d406de295c551 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.
+// 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.
-// 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..fba3d96b4389f 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 creation order. A later report of an
+// identity replaces the earlier one and takes its own position.
+TEST(Remark, TestRemarkFinalOrder) {
+ std::vector<std::string> emitted;
+ {
+ MLIRContext context;
+ ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
+ Location locA = FileLineColLoc::get(&context, "test.cpp", 2, 5);
+ Location locB = FileLineColLoc::get(&context, "test.cpp", 1, 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: B", "Unroller: A last"));
+}
+
// 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,8 @@ 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"));
}
// A RelatedTo link only resolves between remarks drained by the same
More information about the Mlir-commits
mailing list