[Mlir-commits] [mlir] [MLIR] Drain remarks in RemarkEmittingPolicyFinal::finalize() (PR #224607)
Henrich Lauko
llvmlistbot at llvm.org
Fri Sep 18 04:58:12 PDT 2026
https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/224607
>From eefef406becd6d73574b58a60ec2ec783dbc47b0 Mon Sep 17 00:00:00 2001
From: Henrich Lauko <hlauko at nvidia.com>
Date: Fri, 18 Sep 2026 11:38:51 +0000
Subject: [PATCH] [MLIR][Remark] Drain remarks in
RemarkEmittingPolicyFinal::finalize()
RemarkEmittingPolicyFinal::finalize() iterated postponedRemarks without
clearing it. MlirOptMain calls finalize() after printing the module and
~RemarkEngine calls it again when the context is destroyed, so a YAML or
bitstream streamer serialized every remark twice. With
mlir/test/Pass/remark-final.mlir the YAML file held 10 records for 5
remarks. The console path hid the duplicates because the diagnostic
handler is gone by the time the destructor runs.
Swap the pending set into a local before emitting. A second call then
emits only remarks reported since the first, and a RelatedTo link
resolves only between remarks drained by the same call. Both are
documented in the header and in Remarks.md and covered by unit tests.
The lit test now pins the record count with --implicit-check-not.
The drain was first applied downstream by Jhalak Patel.
Assisted-by: Claude Code (Claude Fable 5.1)
---
mlir/docs/Remarks.md | 6 +-
mlir/include/mlir/IR/Remarks.h | 13 +++--
mlir/lib/IR/Remarks.cpp | 9 ++-
mlir/test/Pass/remark-final.mlir | 26 +++++----
mlir/unittests/IR/RemarkTest.cpp | 96 ++++++++++++++++++++++++++++++++
5 files changed, 130 insertions(+), 20 deletions(-)
diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md
index c70e20cd44ec4..3f468e730003d 100644
--- a/mlir/docs/Remarks.md
+++ b/mlir/docs/Remarks.md
@@ -202,8 +202,10 @@ Emits **all** remarks unconditionally.
### RemarkEmittingPolicyFinal
-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.
+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:
diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 81073efa4e026..f0cdedbadb7e7 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -665,12 +665,11 @@ class RemarkEmittingPolicyAll : public detail::RemarkEmittingPolicyBase {
void finalize() override {}
};
-/// Policy that emits final remarks. Stores all remarks until finalize(),
-/// which enables query-based linking via findRemarks().
+/// Policy that emits only the last remark reported for each identity, see
+/// DenseMapInfo<Remark>. Remarks are stored until finalize().
class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
private:
- /// user can intercept them for custom processing via a registered callback,
- /// otherwise they will be reported on engine destruction.
+ /// Remarks reported since the last finalize().
llvm::DenseSet<detail::Remark> postponedRemarks;
public:
@@ -681,8 +680,10 @@ class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
postponedRemarks.insert(remark);
}
- /// Emits all stored remarks. Related remarks are printed as nested notes
- /// under the remark that references them.
+ /// 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.
void finalize() override;
};
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index 85c19dbd89250..57e43b8a0d090 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -368,11 +368,16 @@ RemarkEmittingPolicyFinal::RemarkEmittingPolicyFinal() = default;
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);
+
// 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 : postponedRemarks) {
+ for (const auto &remark : remarks) {
if (remark.getId())
idMap[remark.getId().getValue()] = &remark;
for (auto relId : remark.getRelatedRemarkIds())
@@ -383,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 : postponedRemarks) {
+ for (const auto &remark : 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 325271e04cc5c..acf768e6e51f8 100644
--- a/mlir/test/Pass/remark-final.mlir
+++ b/mlir/test/Pass/remark-final.mlir
@@ -1,17 +1,23 @@
-// RUN: mlir-opt %s --test-remark --remarks-filter="category.*" --remark-policy=final 2>&1 | FileCheck %s
+// RUN: mlir-opt %s --test-remark --remarks-filter="category.*" --remark-policy=final 2>&1 | FileCheck %s --implicit-check-not="remark:" --implicit-check-not="should be dropped"
// RUN: mlir-opt %s --test-remark --remarks-filter="category.*" --remark-policy=final --remark-format=yaml --remarks-output-file=%t.yaml
-// RUN: FileCheck --check-prefix=CHECK-YAML %s < %t.yaml
+// RUN: FileCheck --check-prefix=CHECK-YAML %s --implicit-check-not="--- !" --implicit-check-not="should be dropped" < %t.yaml
module @foo {
"test.op"() : () -> ()
-
}
-// CHECK-YAML-NOT: This is a test passed remark (should be dropped)
-// CHECK-YAML-DAG: !Analysis
-// CHECK-YAML-DAG: !Failure
-// CHECK-YAML-DAG: !Passed
+// 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-NOT: This is a test passed remark (should be dropped)
-// CHECK-DAG: remark: [Analysis] test-remark
+// 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: [Passed] test-remark | Category:category-1-passed
+// 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-YAML-DAG: --- !Passed
+// CHECK-YAML-DAG: --- !Failure
+// CHECK-YAML-DAG: --- !Analysis
+// CHECK-YAML-DAG: --- !Passed
+// CHECK-YAML-DAG: --- !Analysis
diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp
index c02f1e1c14bf5..325d7f24c8aa4 100644
--- a/mlir/unittests/IR/RemarkTest.cpp
+++ b/mlir/unittests/IR/RemarkTest.cpp
@@ -22,6 +22,7 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <optional>
+#include <vector>
using namespace mlir;
using namespace testing;
@@ -408,6 +409,101 @@ TEST(Remark, TestRemarkFinal) {
std::string::npos); // shown (passed, diff loc)
}
+/// Streamer that records "<remark name>: <Remark arg>" for every remark it
+/// receives, 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())
+ if (arg.key == "Remark")
+ entry += ": " + arg.val;
+ out.push_back(std::move(entry));
+ }
+
+private:
+ std::vector<std::string> &out;
+};
+
+static LogicalResult enableFinalPolicy(MLIRContext &context,
+ std::vector<std::string> &emitted,
+ StringRef category) {
+ mlir::remark::RemarkCategories cats{/*all=*/std::nullopt,
+ /*passed=*/category.str(),
+ /*missed=*/std::nullopt,
+ /*analysis=*/category.str(),
+ /*failed=*/std::nullopt};
+ return remark::enableOptimizationRemarks(
+ context, std::make_unique<RecordingStreamer>(emitted),
+ std::make_unique<remark::RemarkEmittingPolicyFinal>(), cats,
+ /*printAsEmitRemarks=*/false);
+}
+
+// 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
+// again for the next.
+TEST(Remark, TestRemarkFinalDrains) {
+ std::vector<std::string> emitted;
+ {
+ MLIRContext context;
+ ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
+ Location loc = FileLineColLoc::get(&context, "test.cpp", 1, 5);
+ auto *policy = context.getRemarkEngine()->getRemarkEmittingPolicy();
+ auto first = remark::RemarkOpts::name("First").category("LoopUnroll");
+ auto second = remark::RemarkOpts::name("Second").category("LoopUnroll");
+
+ remark::passed(loc, first) << "first";
+ remark::passed(loc, second) << "second";
+ policy->finalize();
+ EXPECT_THAT(emitted,
+ UnorderedElementsAre("First: first", "Second: second"));
+
+ // Nothing pending: a repeated call emits nothing.
+ policy->finalize();
+ EXPECT_EQ(emitted.size(), 2u);
+
+ // A drained identity can be reported again; it waits for the next call,
+ // which here is the engine destructor's.
+ remark::passed(loc, first) << "first again";
+ EXPECT_EQ(emitted.size(), 2u);
+ }
+ ASSERT_EQ(emitted.size(), 3u);
+ EXPECT_EQ(emitted[2], "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.
+TEST(Remark, TestRemarkFinalLinkAcrossDrains) {
+ std::vector<std::string> emitted;
+ {
+ 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(
+ loc, remark::RemarkOpts::name("Analysis").category("LoopUnroll"));
+ analysis << "trip count 128";
+ analysisId = analysis.getId();
+ }
+ policy->finalize();
+ EXPECT_THAT(emitted, ElementsAre("Analysis: trip count 128"));
+
+ remark::passed(loc, remark::RemarkOpts::name("Unroller")
+ .category("LoopUnroll")
+ .relatedTo(analysisId))
+ << "unrolled";
+ }
+ EXPECT_THAT(emitted,
+ ElementsAre("Analysis: trip count 128", "Unroller: unrolled"));
+}
+
TEST(Remark, TestArgWithAttribute) {
MLIRContext context;
More information about the Mlir-commits
mailing list