[MLIR] Add option to postpone remark emission

[grypp]

By default, InFlightRemark are emitted as soon as their scope ends.

This PR adds a mode to postpone emission until the end of compilation, so remarks are processed at once.

It also provides getPostponedRemarks function. So if it's desired the compiler can postpone all the remarks until the end, then sort it or do something with them, and show it to the user.

Future work:
This feature will allow later passes to drop, rewrite, or aggregate remarks before they are printed/streamed. This will be done in follow-up PRs.

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Guray Ozen (grypp)

Changes

By default, InFlightRemark are emitted as soon as their scope ends.

This PR adds a mode to postpone emission until the end of compilation, so remarks are processed at once.

This feature will allow later passes to drop, rewrite, or aggregate remarks before they are printed/streamed. This will be done in follow-up PRs.

---

Full diff: https://github.com/llvm/llvm-project/pull/157434.diff

3 Files Affected:

• (modified) mlir/include/mlir/IR/Remarks.h (+21-6)
• (modified) mlir/lib/IR/Remarks.cpp (+15-1)
• (modified) mlir/unittests/IR/RemarkTest.cpp (+87)

diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 26d65472f2b1c..8879a5f5bb662 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -60,22 +60,27 @@ struct RemarkOpts {
   StringRef categoryName;    // Category name (subject to regex filtering)
   StringRef subCategoryName; // Subcategory name
   StringRef functionName;    // Function name if available
+  bool postponed = false;    // Postpone showing the remark
 
   // Construct RemarkOpts from a remark name.
   static constexpr RemarkOpts name(StringRef n) {
-    return RemarkOpts{n, {}, {}, {}};
+    return RemarkOpts{n, {}, {}, {}, false};
   }
   /// Return a copy with the category set.
   constexpr RemarkOpts category(StringRef v) const {
-    return {remarkName, v, subCategoryName, functionName};
+    return {remarkName, v, subCategoryName, functionName, postponed};
   }
   /// Return a copy with the subcategory set.
   constexpr RemarkOpts subCategory(StringRef v) const {
-    return {remarkName, categoryName, v, functionName};
+    return {remarkName, categoryName, v, functionName, postponed};
   }
   /// Return a copy with the function name set.
   constexpr RemarkOpts function(StringRef v) const {
-    return {remarkName, categoryName, subCategoryName, v};
+    return {remarkName, categoryName, subCategoryName, v, postponed};
+  }
+  /// Return a copy with the function name set.
+  constexpr RemarkOpts postpone() const {
+    return {remarkName, categoryName, subCategoryName, functionName, true};
   }
 };
 
@@ -92,12 +97,13 @@ class Remark {
          RemarkOpts opts)
       : remarkKind(remarkKind), functionName(opts.functionName), loc(loc),
         categoryName(opts.categoryName), subCategoryName(opts.subCategoryName),
-        remarkName(opts.remarkName) {
+        remarkName(opts.remarkName), postponed(opts.postponed) {
     if (!categoryName.empty() && !subCategoryName.empty()) {
       (llvm::Twine(categoryName) + ":" + subCategoryName)
           .toStringRef(fullCategoryName);
     }
   }
+  ~Remark() = default;
 
   // Remark argument that is a key-value pair that can be printed as machine
   // parsable args.
@@ -168,6 +174,8 @@ class Remark {
 
   StringRef getRemarkTypeString() const;
 
+  bool isPostponed() const { return postponed; }
+
 protected:
   /// Keeps the MLIR diagnostic kind, which is used to determine the
   /// diagnostic kind in the LLVM remark streamer.
@@ -191,6 +199,9 @@ class Remark {
   /// Args collected via the streaming interface.
   SmallVector<Arg, 4> args;
 
+  /// Whether the remark is postponed (to be shown later).
+  bool postponed = false;
+
 private:
   /// Convert the MLIR diagnostic severity to LLVM diagnostic severity.
   static llvm::DiagnosticSeverity
@@ -344,6 +355,8 @@ class MLIRRemarkStreamerBase {
 
 class RemarkEngine {
 private:
+  /// Postponed remarks. They are shown at the end of the pipeline.
+  llvm::SmallVector<Remark, 8> postponedRemarks;
   /// Regex that filters missed optimization remarks: only matching one are
   /// reported.
   std::optional<llvm::Regex> missFilter;
@@ -392,6 +405,8 @@ class RemarkEngine {
   InFlightRemark emitIfEnabled(Location loc, RemarkOpts opts,
                                bool (RemarkEngine::*isEnabled)(StringRef)
                                    const);
+  /// Emit all postponed remarks.
+  void emitPostponedRemarks();
 
 public:
   /// Default constructor is deleted, use the other constructor.
@@ -411,7 +426,7 @@ class RemarkEngine {
                            std::string *errMsg);
 
   /// Report a remark.
-  void report(const Remark &&remark);
+  void report(const Remark &remark, bool ignorePostpone = false);
 
   /// Report a successful remark, this will create an InFlightRemark
   /// that can be used to build the remark using the << operator.
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index 78c964427868f..3307be31bec6b 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -225,7 +225,13 @@ InFlightRemark RemarkEngine::emitOptimizationRemarkAnalysis(Location loc,
 // RemarkEngine
 //===----------------------------------------------------------------------===//
 
-void RemarkEngine::report(const Remark &&remark) {
+void RemarkEngine::report(const Remark &remark, bool ignorePostpone) {
+  // Postponed remarks are shown at the end of pipeline, unless overridden.
+  if (remark.isPostponed() && !ignorePostpone) {
+    postponedRemarks.push_back(remark);
+    return;
+  }
+
   // Stream the remark
   if (remarkStreamer)
     remarkStreamer->streamOptimizationRemark(remark);
@@ -235,7 +241,15 @@ void RemarkEngine::report(const Remark &&remark) {
     emitRemark(remark.getLocation(), remark.getMsg());
 }
 
+void RemarkEngine::emitPostponedRemarks() {
+  for (auto &remark : postponedRemarks)
+    report(remark, /*ignorePostpone=*/true);
+  postponedRemarks.clear();
+}
+
 RemarkEngine::~RemarkEngine() {
+  emitPostponedRemarks();
+
   if (remarkStreamer)
     remarkStreamer->finalize();
 }
diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp
index 65e1e08b83838..ca506fcf0c0ed 100644
--- a/mlir/unittests/IR/RemarkTest.cpp
+++ b/mlir/unittests/IR/RemarkTest.cpp
@@ -312,4 +312,91 @@ TEST(Remark, TestCustomOptimizationRemarkDiagnostic) {
   EXPECT_NE(errOut.find(pass2Msg), std::string::npos); // printed
   EXPECT_EQ(errOut.find(pass3Msg), std::string::npos); // filtered out
 }
+
+TEST(Remark, TestCustomOptimizationRemarkPostponeDiagnostic) {
+  testing::internal::CaptureStderr();
+  const auto *pass1Msg = "My message";
+  const auto *pass2Msg = "My another message";
+  const auto *pass3Msg = "Do not show this message";
+
+  std::string categoryLoopunroll("LoopUnroll");
+  std::string categoryInline("Inliner");
+  std::string myPassname1("myPass1");
+  std::string myPassname2("myPass2");
+  std::string funcName("myFunc");
+
+  std::string seenMsg = "";
+
+  {
+    MLIRContext context;
+    Location loc = UnknownLoc::get(&context);
+
+    // Setup the remark engine
+    mlir::remark::RemarkCategories cats{/*passed=*/categoryLoopunroll,
+                                        /*missed=*/std::nullopt,
+                                        /*analysis=*/std::nullopt,
+                                        /*failed=*/categoryLoopunroll};
+
+    LogicalResult isEnabled = remark::enableOptimizationRemarks(
+        context, std::make_unique<MyCustomStreamer>(), cats, true);
+    ASSERT_TRUE(succeeded(isEnabled)) << "Failed to enable remark engine";
+
+    // Remark 1: pass, category LoopUnroll
+    remark::passed(loc, remark::RemarkOpts::name("")
+                            .category(categoryLoopunroll)
+                            .subCategory(myPassname2)
+                            .postpone())
+        << pass1Msg;
+
+    // Postponed remark should not be printed yet.
+    testing::internal::CaptureStderr();
+    llvm::errs().flush();
+    std::string errOut1 = testing::internal::GetCapturedStderr();
+    // Ensure no remark has been printed yet.
+    EXPECT_TRUE(errOut1.empty())
+        << "Expected no stderr output before postponed remarks are flushed";
+
+    // Remark 2: failure, category LoopUnroll
+    remark::failed(loc, remark::RemarkOpts::name("")
+                            .category(categoryLoopunroll)
+                            .subCategory(myPassname2)
+                            .postpone())
+        << remark::reason(pass2Msg);
+
+    // Postponed remark should not be printed yet.
+    testing::internal::CaptureStderr();
+    llvm::errs().flush();
+    std::string errOut2 = testing::internal::GetCapturedStderr();
+    // Ensure no remark has been printed yet.
+    EXPECT_TRUE(errOut2.empty())
+        << "Expected no stderr output before postponed remarks are flushed";
+
+    // Remark 3: pass, category Inline (should not be printed)
+    remark::passed(loc, remark::RemarkOpts::name("")
+                            .category(categoryInline)
+                            .subCategory(myPassname1))
+        << pass3Msg;
+
+    testing::internal::CaptureStderr();
+    llvm::errs().flush();
+    std::string errOut = testing::internal::GetCapturedStderr();
+    auto third = errOut.find("Custom remark:");
+    EXPECT_EQ(third, std::string::npos);
+  }
+  testing::internal::CaptureStderr();
+  llvm::errs().flush();
+  std::string errOut = ::testing::internal::GetCapturedStderr();
+
+  // Expect exactly two "Custom remark:" lines.
+  auto first = errOut.find("Custom remark:");
+  EXPECT_NE(first, std::string::npos);
+  auto second = errOut.find("Custom remark:", first + 1);
+  EXPECT_NE(second, std::string::npos);
+
+  // Containment checks for messages.
+  EXPECT_NE(errOut.find(pass1Msg), std::string::npos); // printed
+  EXPECT_NE(errOut.find(pass2Msg), std::string::npos); // printed
+  EXPECT_EQ(errOut.find(pass3Msg), std::string::npos); // filtered out
+}
+
 } // namespace

[Copilot]

Pull Request Overview

This PR adds functionality to postpone remark emission in MLIR until the end of compilation, allowing remarks to be processed in batches rather than immediately upon creation. This enables future optimization passes to drop, rewrite, or aggregate remarks before they are emitted.

• Adds a postponed field to RemarkOpts and Remark classes to control emission timing
• Modifies RemarkEngine to store postponed remarks and emit them during destruction
• Implements a new postpone() method in the fluent API for marking remarks as postponed

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
mlir/include/mlir/IR/Remarks.h	Adds postponed field and methods to Remark classes and RemarkEngine
mlir/lib/IR/Remarks.cpp	Implements postponed remark storage and emission logic
mlir/unittests/IR/RemarkTest.cpp	Adds comprehensive test for postponed remark functionality

[grypp]

ping for review @joker-eph @matthias-springer

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[razvanlupusoru]

Thank you Guray for your continued contributions in this area. The functionality to postpone remarks is great as I want this as well :) However, it feels a bit strange that "postponed" is a property of the Remark itself - it seems more natural if this was a setting of the RemarkEngine to collect everything until either "reportAll" is called or RemarkEngine is destroyed.

[grypp]

Thank you Guray for your continued contributions in this area. The functionality to postpone remarks is great as I want this as well :) However, it feels a bit strange that "postponed" is a property of the Remark itself - it seems more natural if this was a setting of the RemarkEngine to collect everything until either "reportAll" is called or RemarkEngine is destroyed.

Hmm, are you suggesting making postponing part of the RemarkEngine and applying it to every remark? We can have this. But having this only on the RemarkEngine is coarse, then you wouldn’t be able to differentiate which remarks to postpone and which not to.

Postponing remarks can be costly because the RemarkEngine has to keep them in memory. So I thought if you know a remark won’t change, you shouldn’t postpone it.

How is your use-case? For Flang, do you want to postpone every remark?

[joker-eph]

Hmm, are you suggesting making postponing part of the RemarkEngine and applying it to every remark? We can have this. But having this only on the RemarkEngine is coarse, then you wouldn’t be able to differentiate which remarks to postpone and which not to.

Not if it is a callback and the user can dynamically decide what they want to emit or postpone.

Actually: can this be layered so that the Engine always forward, but the client intercept with another layer?

[grypp]

RemarkEngine has a callback. It calls MLIRRemarkStreamerBase that's an abstract class, and one can implement custom streamer.

You can configure this callback as shown below. In that case, you don’t need to call postpone() when printing the remark—you can implement your own postponing mechanism directly in the streamer.

class MyPostponer : public remark::detail::MLIRRemarkStreamerBase {
public:
  DenseSet<Remark> remarks;
  void streamOptimizationRemark(const remark::detail::Remark &remark) override {
    remarks.insert(remark);
  }
  ~MyPostponer() {
    for (const auto &remark : remarks) {
      // sort remarks and etc.
      print(remark)
    }
  }
};

// Configure with the postponer
remark::enableOptimizationRemarks(context, std::make_unique<MyPostponer>(), ...);

[joker-eph]

Right, so back to the question: I still don't see why the emitter of the remark should know about this, and as such I don't understand why the concept of "postpone" belongs to the remark itself.

[grypp]

Right, so back to the question: I still don't see why the emitter of the remark should know about this,

At the point of emission, you know whether the Remark should be postponed or not — for example, if you know a later pass will attempt to optimize this op again. If postponing is handled entirely in the callback, how would it know which remarks to postpone and which not to?

If I’m missing something, could you show me a draft of the code? I’d be happy to adjust my approach.

and as such I don't understand why the concept of "postpone" belongs to the remark itself.
fwiw, this PR makes postponing a trait of RemarkEngine, not the Remark class.

My impression is that @razvanlupusoru wants to postpone every remark by default — if that’s the case, we could add a postponeAll flag to RemarkEngine.

[joker-eph]

At the point of emission, you know whether the Remark should be postponed or not — for example, if you know a later pass will attempt to optimize this op again.

But... this example is exactly why I don't believe that the point of emission is right! That would be coupling a utility trying to do something to a specific pass pipeline!

[razvanlupusoru]

At the point of emission, you know whether the Remark should be postponed or not — for example, if you know a later pass will attempt to optimize this op again. If postponing is handled entirely in the callback, how would it know which remarks to postpone and which not to?

For my use case, I plan to sort and group so that all messages for a specific line number are together. The pass emitting the remark doesn't necessarily know I need to do that so it is surprising it would be the one controlling whether the remark it emits gets postponed or not.

Also, I can see cases where the pass emitting the remark won't know it is finalized. One example in OpenACC pipeline is that we want messages emitted for when we decide to do an implicit copy to GPU. However, we are planning for a follow-up pass to finetune and add bounds based on array range use for that copy. I do not see how the pass emitting the remark can know whether the message is final or might need dropped/replaced later.

My impression is that @razvanlupusoru wants to postpone every remark by default — if that’s the case, we could add a postponeAll flag to RemarkEngine.

Yes I do - I want to collect all remarks to sort them and group them- and I can only do that once I have all of them and all are "postponed".

And thank you again for working on this functionality Guray! I appreciate that you are creating a framework that can be useful for others :)

[grypp]

Let me illustrate my use case — I believe it’s similar to @razvanlupusoru’s OpenACC case and is actually a very typical compiler diagnostic scenario.

In this example, Remark-1 is incorrect, because pass-late actually unrolled the loop.

    [pass-early] : emit Remark-1: failed unrolling, postponed
    ... other passes ...
    [pass-late]  : emit Remark-2: succeeded unrolling // same location/op

Idea 1 (current approach):
Steps:

1. pass-early : emits the Remark-1 as postponed
2. pass-late : drop Remark-1 and emit Remark-2

Dropping postponed remarks is implemented in a follow-up PR #159519, please take a look.

Advantage: Passes have full control over which remarks to keep, drop, or replace.
Disadvantage: Pass developers need to think what to postpone, and they should know the entire pipeline to design diagnostics correctly.

Idea 2:
Add an option like --remark-policy=all|final

• all : shows Remark-1 and Remark-2.
• final : RemarkEngine postpones all remarks by default, and automatically drops earlier ones, keeping only the last remark per location/op.

Advantage: Simpler for passes — no need to manage drops manually.
Disadvantage: If you don't want to drop the remarks in some places, you have no control.

[razvanlupusoru]

Idea 1 does not make sense to me because of my impression that users of MLIR will build their compilers with different pass pipelines. So the disadvantage you captured seems to be an intractable point:

Disadvantage: Pass developers need to think what to postpone, and they should know the entire pipeline to design diagnostics correctly.

Back to OpenACC - we will support both Clang (CIR) and Flang. They will each have their own pass pipelines with a different order and use of MLIR OpenACC passes. It is not realistic that a pass itself knows anything about the pipeline; Mehdi captured this concern well: "That would be coupling a utility trying to do something to a specific pass pipeline!"

Thus Idea 2 seems like the better option. I don't think I grasp what the command line options has to do with the postponing aspect - but at least from my perspective I don't see idea 2 as being disruptive to how I am hoping to use the remarks framework. And I am indeed interested in seeing something implemented that will allow me to collect all Remarks instead of printing them when InFlightRemark gets destroyed.

[grypp]

#160062 implements the Idea 2

[razvanlupusoru]

#160062 implements the Idea 2

Thanks, Guray! I appreciate your work on implementing Idea 2 in PR #160062.

After reviewing the implementation, it turns out that neither of the proposed policies fits my requirements:

• The "all" policy doesn't work for me because I need all remarks collected and postponed for later sorting, and this policy doesn't support postponing.
• The "final" policy also falls short because, besides the postponing aspect, it implements a dropping policy per location (and category). I need to report multiple messages for the same location and category. For instance, with acc.parallel, we associate messages related to implicit copies and launch configurations with the same location representing the acc region.

Essentially, I need an "all_postponed" policy.

I understand that you need PR #160062 to proceed, and I am okay with moving forward with the implementation of Idea 2, as long as we keep in mind Mehdi's point is likely the proper way to implement this:

Not if it is a callback and the user can dynamically decide what they want to emit or postpone.
Actually: can this be layered so that the Engine always forwards, but the client intercepts with another layer?

This approach aligns more closely with my vision for how message handling, postponing, and dropping should be managed, rather than the proposed Idea 2.

[joker-eph]

I just skimmed here, but can we make the policy pluggable instead of part of the engine?
That is create a virtual base class "RemarkEmittingPolicy" that can be subclassed and setup on the engine, in a way that @razvanlupusoru can implement their own scheme.

[grypp]

Sounds good. I can create pluggable policy class and create two default policies: all, final.

@razvanlupusoru fwiw, for final policy, we can use subcategory match as well. So you can emit multiple remark for 'acc parallel'

[grypp]

I just skimmed here, but can we make the policy pluggable instead of part of the engine? That is create a virtual base class "RemarkEmittingPolicy" that can be subclassed and setup on the engine, in a way that @razvanlupusoru can implement their own scheme.

#160526 implements that

[joker-eph]

Is this to be closed now that #160526 is merged?