[llvm-branch-commits] [mlir] [MLIR][Remark] Move reported remarks into the policy (PR #227696)

[Henrich Lauko via llvm-branch-commits]

https://github.com/xlauko created https://github.com/llvm/llvm-project/pull/227696

Stacked on #227360 (base branch `users/xlauko/mlir-remark-thread-safe`).

**Problem.** `RemarkEngine::report(const Remark &&)` takes a const rvalue, so the `std::move` in `InFlightRemark::~InFlightRemark` never moves and the policy sees a `const Remark &`. `RemarkEmittingPolicyFinal::reportRemark` then copies the whole `Remark` (four `std::string`s, a `SmallString<64>`, a `SmallVector<Arg, 4>` with 80-byte elements, a `SmallVector<RemarkId>`; 640 bytes in total) into its `DenseSet` on every report, and since #227360 that copy runs under the engine lock.

**Change.** `report()` and `RemarkEmittingPolicyBase::reportRemark()` take `Remark &&`; the final policy moves the remark into its set (`DenseSet::insert(ValueT &&)` forwards to `DenseMap::try_emplace(KeyT &&)`, which finishes the lookup before move-constructing the key). The in-flight remark owns the object and destroys it right after reporting, so nothing observes the moved-from state. The All policy still just forwards a reference to the streamer. A temporary copy-counting probe confirmed zero `Remark` copies on the report path in the Remark unit tests.

**Behaviour.** Emitted remarks and their order are unchanged. Out-of-tree custom policies need to change their `reportRemark` override to take `Remark &&`; the `RemarkEmittingPolicyBase` comment now states the ownership rule.


>From fe78836c40af3ef7003f0aad737bb346f0f89347 Mon Sep 17 00:00:00 2001
From: Henrich Lauko <hlauko at nvidia.com>
Date: Wed, 30 Sep 2026 12:06:47 +0000
Subject: [PATCH] [MLIR][Remark] Move reported remarks into the policy

RemarkEngine::report took a const rvalue reference, so the std::move in
InFlightRemark's destructor never moved anything and the policy received
a const reference. RemarkEmittingPolicyFinal then copied the whole Remark,
its strings and argument vector included, into its DenseSet on every
report, and since the engine lock was added it did so inside the critical
section.

Take the remark by non-const rvalue reference in report() and
reportRemark(), and move it into the final policy's set. The in-flight
remark owns the object and destroys it right after reporting, so nothing
observes the moved-from state. Emitted output is unchanged. Out-of-tree
policies need to change their reportRemark signature to Remark &&.

Assisted-by: Claude Code (Claude Fable 5.1)
---
 mlir/include/mlir/IR/Remarks.h | 13 +++++++------
 mlir/lib/IR/Remarks.cpp        |  4 ++--
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 97eba18690a13..3306533f4fa66 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -472,7 +472,8 @@ using ReportFn = llvm::unique_function<void(const Remark &)>;
 /// Base class for MLIR remark emitting policies that is used to emit
 /// optimization remarks to the underlying remark streamer. The derived classes
 /// should implement the `reportRemark` method to provide the actual emitting
-/// implementation.
+/// implementation. `reportRemark` owns the remark it receives; a policy that
+/// keeps it past the call must move it into its own storage.
 ///
 /// Through the RemarkEngine, `reportRemark` and `finalize` run under the
 /// engine's lock and are never entered concurrently, even when passes report
@@ -487,7 +488,7 @@ class RemarkEmittingPolicyBase {
 
   void initialize(ReportFn fn) { reportImpl = std::move(fn); }
 
-  virtual void reportRemark(const Remark &remark) = 0;
+  virtual void reportRemark(Remark &&remark) = 0;
   virtual void finalize() = 0;
 
   /// Find previously reported remarks matching the given criteria.
@@ -629,7 +630,7 @@ class RemarkEngine {
 
   /// Report a remark. Thread-safe: reports from several threads are handed to
   /// the policy one at a time.
-  void report(const Remark &&remark);
+  void report(Remark &&remark);
 
   /// Report a successful remark, this will create an InFlightRemark
   /// that can be used to build the remark using the << operator.
@@ -679,7 +680,7 @@ class RemarkEmittingPolicyAll : public detail::RemarkEmittingPolicyBase {
 public:
   RemarkEmittingPolicyAll();
 
-  void reportRemark(const detail::Remark &remark) override {
+  void reportRemark(detail::Remark &&remark) override {
     assert(reportImpl && "reportImpl is not set");
     reportImpl(remark);
   }
@@ -697,9 +698,9 @@ class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
 public:
   RemarkEmittingPolicyFinal();
 
-  void reportRemark(const detail::Remark &remark) override {
+  void reportRemark(detail::Remark &&remark) override {
     postponedRemarks.erase(remark);
-    postponedRemarks.insert(remark);
+    postponedRemarks.insert(std::move(remark));
   }
 
   /// Emits and drains all stored remarks. Root remarks come out sorted by the
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index 9d9a27edb5216..eb5bbb0e9694f 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -262,11 +262,11 @@ void RemarkEngine::reportImpl(const Remark &remark) {
     emitRemark(remark.getLocation(), remark.getMsg());
 }
 
-void RemarkEngine::report(const Remark &&remark) {
+void RemarkEngine::report(Remark &&remark) {
   if (!remarkEmittingPolicy)
     return;
   llvm::sys::SmartScopedLock<true> lock(mutex);
-  remarkEmittingPolicy->reportRemark(remark);
+  remarkEmittingPolicy->reportRemark(std::move(remark));
 }
 
 void RemarkEngine::finalizePolicy() {