[Mlir-commits] [mlir] c02e5de - [MLIR] Drain remarks in RemarkEmittingPolicyFinal::finalize() (#224607)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Fri Sep 18 05:54:48 PDT 2026
Author: Henrich Lauko
Date: 2026-09-18T14:54:41+02:00
New Revision: c02e5de301c6cb20ba5d0942b71ad8c2b412fb99
URL: https://github.com/llvm/llvm-project/commit/c02e5de301c6cb20ba5d0942b71ad8c2b412fb99
DIFF: https://github.com/llvm/llvm-project/commit/c02e5de301c6cb20ba5d0942b71ad8c2b412fb99.diff
LOG: [MLIR] Drain remarks in RemarkEmittingPolicyFinal::finalize() (#224607)
`RemarkEmittingPolicyFinal::finalize()` iterates `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 serializes every remark
twice. With `mlir/test/Pass/remark-final.mlir` the YAML output file
holds 10 records for 5 remarks. The console path hides this because the
diagnostic handler is already gone when the destructor runs.
This change swaps the pending set into a local before emitting. A second
`finalize()` then emits only the remarks reported since the first one. A
consequence worth stating: a `RelatedTo` link resolves only between
remarks drained by the same call. Both behaviours are documented in the
header comment and in `Remarks.md`.
A follow-up will make the emission order of the final policy
deterministic; today it is `DenseSet` iteration order, which is why the
lit test keeps `CHECK-DAG`.
Assisted-by: Claude Code (Claude Fable 5.1). I read and reviewed the
change before opening it.
Added:
Modified:
mlir/docs/Remarks.md
mlir/include/mlir/IR/Remarks.h
mlir/lib/IR/Remarks.cpp
mlir/test/Pass/remark-final.mlir
mlir/unittests/IR/RemarkTest.cpp
Removed:
################################################################################
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