[MLIR] Introduce RemarkPolicy for enhanced remark handling

mlir/include/mlir/IR/Remarks.h

@@ -32,6 +32,20 @@ struct RemarkCategories {
   std::optional<std::string> all, passed, missed, analysis, failed;
 };
 
+/// Define the policy to use for the remark engine.
+enum RemarkPolicy {
+  // Show all remarks
+  RemarkPolicyAll = 0,
+  // Show only the last remark per location, remark name and category
+  RemarkPolicyFinal = 1,
+};
+
+/// Options to create a RemarkEngine.
+struct RemarkEngineOpts {
+  RemarkCategories categories;
+  RemarkPolicy policy;
+};
+
 /// Categories describe the outcome of an transformation, not the mechanics of
 /// emitting/serializing remarks.
 enum class RemarkKind {
@@ -144,7 +158,7 @@ class Remark {
 
   llvm::StringRef getCategoryName() const { return categoryName; }
 
-  llvm::StringRef getFullCategoryName() const {
+  llvm::StringRef getCombinedCategoryName() const {
     if (categoryName.empty() && subCategoryName.empty())
       return {};
     if (subCategoryName.empty())
@@ -344,6 +358,10 @@ class MLIRRemarkStreamerBase {
 
 class RemarkEngine {
 private:
+  /// Postponed remarks. They are deferred to the end of the pipeline, where the
+  /// user can intercept them for custom processing, otherwise they will be
+  /// reported on engine destruction.
+  llvm::DenseSet<Remark> postponedRemarks;
   /// Regex that filters missed optimization remarks: only matching one are
   /// reported.
   std::optional<llvm::Regex> missFilter;
@@ -357,6 +375,8 @@ class RemarkEngine {
   std::unique_ptr<MLIRRemarkStreamerBase> remarkStreamer;
   /// When is enabled, engine also prints remarks as mlir::emitRemarks.
   bool printAsEmitRemarks = false;
+  /// The policy to use for the remark engine.
+  RemarkPolicy remarkPolicy = RemarkPolicy::RemarkPolicyAll;
 
   /// Return true if missed optimization remarks are enabled, override
   /// to provide different implementation.
@@ -392,6 +412,11 @@ class RemarkEngine {
   InFlightRemark emitIfEnabled(Location loc, RemarkOpts opts,
                                bool (RemarkEngine::*isEnabled)(StringRef)
                                    const);
+  /// Emit all postponed remarks.
+  void emitPostponedRemarks();
+
+  /// Report a remark.
+  void reportImpl(const Remark &remark);
 
 public:
   /// Default constructor is deleted, use the other constructor.
@@ -400,7 +425,7 @@ class RemarkEngine {
   /// Constructs Remark engine with optional category names. If a category
   /// name is not provided, it is not enabled. The category names are used to
   /// filter the remarks that are emitted.
-  RemarkEngine(bool printAsEmitRemarks, const RemarkCategories &cats);
+  RemarkEngine(bool printAsEmitRemarks, const RemarkEngineOpts &opts);
 
   /// Destructor that will close the output file and reset the
   /// main remark streamer.
@@ -410,7 +435,9 @@ class RemarkEngine {
   LogicalResult initialize(std::unique_ptr<MLIRRemarkStreamerBase> streamer,
                            std::string *errMsg);
 
-  /// Report a remark.
+  /// Report a remark. The remark is deferred to the end of the pipeline, where
+  /// the user can intercept them for custom processing, otherwise they will be
+  /// reported on engine destruction.
   void report(const Remark &&remark);
 
   /// Report a successful remark, this will create an InFlightRemark
@@ -513,8 +540,61 @@ inline detail::InFlightRemark analysis(Location loc, RemarkOpts opts) {
 LogicalResult enableOptimizationRemarks(
     MLIRContext &ctx,
     std::unique_ptr<remark::detail::MLIRRemarkStreamerBase> streamer,
-    const remark::RemarkCategories &cats, bool printAsEmitRemarks = false);
+    const remark::RemarkEngineOpts &opts, bool printAsEmitRemarks = false);
 
 } // namespace mlir::remark
 
+// DenseMapInfo specialization for Remark
+namespace llvm {
+template <>
+struct DenseMapInfo<mlir::remark::detail::Remark> {
+  static constexpr StringRef kEmptyKey = "<EMPTY_KEY>";
+  static constexpr StringRef kTombstoneKey = "<TOMBSTONE_KEY>";
+
+  /// Helper to provide a static dummy context for sentinel keys.
+  static mlir::MLIRContext *getStaticDummyContext() {
+    static mlir::MLIRContext dummyContext;
+    return &dummyContext;
+  }
+
+  /// Create an empty remark
+  static inline mlir::remark::detail::Remark getEmptyKey() {
+    return mlir::remark::detail::Remark(
+        mlir::remark::RemarkKind::RemarkUnknown, mlir::DiagnosticSeverity::Note,
+        mlir::UnknownLoc::get(getStaticDummyContext()),
+        mlir::remark::RemarkOpts::name(kEmptyKey));
+  }
+
+  /// Create a dead remark
+  static inline mlir::remark::detail::Remark getTombstoneKey() {
+    return mlir::remark::detail::Remark(
+        mlir::remark::RemarkKind::RemarkUnknown, mlir::DiagnosticSeverity::Note,
+        mlir::UnknownLoc::get(getStaticDummyContext()),
+        mlir::remark::RemarkOpts::name(kTombstoneKey));
+  }
+
+  /// Compute the hash value of the remark
+  static unsigned getHashValue(const mlir::remark::detail::Remark &remark) {
+    return llvm::hash_combine(remark.getLocation().getAsOpaquePointer(),
+                              llvm::hash_value(remark.getRemarkName()),
+                              llvm::hash_value(remark.getCategoryName()));
+  }
+
+  static bool isEqual(const mlir::remark::detail::Remark &lhs,
+                      const mlir::remark::detail::Remark &rhs) {
+    // Check for empty/tombstone keys first
+    if (lhs.getRemarkName() == kEmptyKey ||
+        lhs.getRemarkName() == kTombstoneKey ||
+        rhs.getRemarkName() == kEmptyKey ||
+        rhs.getRemarkName() == kTombstoneKey) {
+      return lhs.getRemarkName() == rhs.getRemarkName();
+    }
+
+    // For regular remarks, compare key identifying fields
+    return lhs.getLocation() == rhs.getLocation() &&
+           lhs.getRemarkName() == rhs.getRemarkName() &&
+           lhs.getCategoryName() == rhs.getCategoryName();
+  }
+};
+} // namespace llvm
 #endif // MLIR_IR_REMARKS_H

mlir/include/mlir/Remark/RemarkStreamer.h

@@ -45,6 +45,6 @@ namespace mlir::remark {
 /// mlir::emitRemarks.
 LogicalResult enableOptimizationRemarksWithLLVMStreamer(
     MLIRContext &ctx, StringRef filePath, llvm::remarks::Format fmt,
-    const RemarkCategories &cat, bool printAsEmitRemarks = false);
+    const RemarkEngineOpts &opts, bool printAsEmitRemarks = false);
 
 } // namespace mlir::remark

mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h

@@ -44,6 +44,11 @@ enum class RemarkFormat {
   REMARK_FORMAT_BITSTREAM,
 };
 
+enum class RemarkPolicy {
+  REMARK_POLICY_ALL,
+  REMARK_POLICY_FINAL,
+};
+
 /// Configuration options for the mlir-opt tool.
 /// This is intended to help building tools like mlir-opt by collecting the
 /// supported options.
@@ -242,6 +247,8 @@ class MlirOptMainConfig {
 
   /// Set the reproducer output filename
   RemarkFormat getRemarkFormat() const { return remarkFormatFlag; }
+  /// Set the remark policy to use.
+  RemarkPolicy getRemarkPolicy() const { return remarkPolicyFlag; }
   /// Set the remark format to use.
   std::string getRemarksAllFilter() const { return remarksAllFilterFlag; }
   /// Set the remark output file.
@@ -265,6 +272,8 @@ class MlirOptMainConfig {
 
   /// Remark format
   RemarkFormat remarkFormatFlag = RemarkFormat::REMARK_FORMAT_STDOUT;
+  /// Remark policy
+  RemarkPolicy remarkPolicyFlag = RemarkPolicy::REMARK_POLICY_ALL;
   /// Remark file to output to
   std::string remarksOutputFileFlag = "";
   /// Remark filters

mlir/lib/IR/MLIRContext.cpp

@@ -278,6 +278,8 @@ class MLIRContextImpl {
     }
   }
   ~MLIRContextImpl() {
+    // finalize remark engine before destroying anything else.
+    remarkEngine.reset();
     for (auto typeMapping : registeredTypes)
       typeMapping.second->~AbstractType();
     for (auto attrMapping : registeredAttributes)

mlir/lib/IR/Remarks.cpp

@@ -70,7 +70,7 @@ static void printArgs(llvm::raw_ostream &os, llvm::ArrayRef<Remark::Arg> args) {
 void Remark::print(llvm::raw_ostream &os, bool printLocation) const {
   // Header: [Type] pass:remarkName
   StringRef type = getRemarkTypeString();
-  StringRef categoryName = getFullCategoryName();
+  StringRef categoryName = getCombinedCategoryName();
   StringRef name = remarkName;
 
   os << '[' << type << "] ";
@@ -140,7 +140,7 @@ llvm::remarks::Remark Remark::generateRemark() const {
   r.RemarkType = getRemarkType();
   r.RemarkName = getRemarkName();
   // MLIR does not use passes; instead, it has categories and sub-categories.
-  r.PassName = getFullCategoryName();
+  r.PassName = getCombinedCategoryName();
   r.FunctionName = getFunction();
   r.Loc = locLambda();
   for (const Remark::Arg &arg : getArgs()) {
@@ -225,7 +225,7 @@ InFlightRemark RemarkEngine::emitOptimizationRemarkAnalysis(Location loc,
 // RemarkEngine
 //===----------------------------------------------------------------------===//
 
-void RemarkEngine::report(const Remark &&remark) {
+void RemarkEngine::reportImpl(const Remark &remark) {
   // Stream the remark
   if (remarkStreamer)
     remarkStreamer->streamOptimizationRemark(remark);
@@ -235,7 +235,25 @@ void RemarkEngine::report(const Remark &&remark) {
     emitRemark(remark.getLocation(), remark.getMsg());
 }
 
+void RemarkEngine::report(const Remark &&remark) {
+  // Postponed remarks are deferred to the end of pipeline.
+  if (remarkPolicy == RemarkPolicy::RemarkPolicyFinal) {
+    postponedRemarks.erase(remark);
+    postponedRemarks.insert(remark);
+    return;
+  }
+
+  reportImpl(remark);
+}
+
+void RemarkEngine::emitPostponedRemarks() {
+  for (auto &remark : postponedRemarks)
+    reportImpl(remark);
+  postponedRemarks.clear();
+}
+
 RemarkEngine::~RemarkEngine() {
+  emitPostponedRemarks();
   if (remarkStreamer)
     remarkStreamer->finalize();
 }
@@ -288,24 +306,25 @@ buildFilter(const mlir::remark::RemarkCategories &cats,
 }
 
 RemarkEngine::RemarkEngine(bool printAsEmitRemarks,
-                           const RemarkCategories &cats)
+                           const RemarkEngineOpts &opts)
     : printAsEmitRemarks(printAsEmitRemarks) {
-  if (cats.passed)
-    passedFilter = buildFilter(cats, cats.passed);
-  if (cats.missed)
-    missFilter = buildFilter(cats, cats.missed);
-  if (cats.analysis)
-    analysisFilter = buildFilter(cats, cats.analysis);
-  if (cats.failed)
-    failedFilter = buildFilter(cats, cats.failed);
+  if (opts.categories.passed)
+    passedFilter = buildFilter(opts.categories, opts.categories.passed);
+  if (opts.categories.missed)
+    missFilter = buildFilter(opts.categories, opts.categories.missed);
+  if (opts.categories.analysis)
+    analysisFilter = buildFilter(opts.categories, opts.categories.analysis);
+  if (opts.categories.failed)
+    failedFilter = buildFilter(opts.categories, opts.categories.failed);
+  remarkPolicy = opts.policy;
 }
 
 llvm::LogicalResult mlir::remark::enableOptimizationRemarks(
     MLIRContext &ctx,
     std::unique_ptr<remark::detail::MLIRRemarkStreamerBase> streamer,
-    const remark::RemarkCategories &cats, bool printAsEmitRemarks) {
+    const remark::RemarkEngineOpts &opts, bool printAsEmitRemarks) {
   auto engine =
-      std::make_unique<remark::detail::RemarkEngine>(printAsEmitRemarks, cats);
+      std::make_unique<remark::detail::RemarkEngine>(printAsEmitRemarks, opts);
 
   std::string errMsg;
   if (failed(engine->initialize(std::move(streamer), &errMsg))) {

mlir/lib/Remark/RemarkStreamer.cpp

@@ -60,14 +60,14 @@ void LLVMRemarkStreamer::finalize() {
 namespace mlir::remark {
 LogicalResult enableOptimizationRemarksWithLLVMStreamer(
     MLIRContext &ctx, StringRef path, llvm::remarks::Format fmt,
-    const RemarkCategories &cat, bool printAsEmitRemarks) {
+    const RemarkEngineOpts &opts, bool printAsEmitRemarks) {
 
   FailureOr<std::unique_ptr<detail::MLIRRemarkStreamerBase>> sOr =
       detail::LLVMRemarkStreamer::createToFile(path, fmt);
   if (failed(sOr))
     return failure();
 
-  return remark::enableOptimizationRemarks(ctx, std::move(*sOr), cat,
+  return remark::enableOptimizationRemarks(ctx, std::move(*sOr), opts,
                                            printAsEmitRemarks);
 }
 

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp

@@ -226,6 +226,18 @@ struct MlirOptMainConfigCLOptions : public MlirOptMainConfig {
                                     "bitstream", "Print bitstream file")),
         llvm::cl::cat(remarkCategory)};
 
+    static llvm::cl::opt<RemarkPolicy, /*ExternalStorage=*/true> remarkPolicy{
+        "remark-policy",
+        llvm::cl::desc("Specify the policy for remark output."),
+        cl::location(remarkPolicyFlag),
+        llvm::cl::value_desc("format"),
+        llvm::cl::init(RemarkPolicy::REMARK_POLICY_ALL),
+        llvm::cl::values(clEnumValN(RemarkPolicy::REMARK_POLICY_ALL, "all",
+                                    "Print all remarks"),
+                         clEnumValN(RemarkPolicy::REMARK_POLICY_FINAL, "final",
+                                    "Print final remarks")),
+        llvm::cl::cat(remarkCategory)};
+
     static cl::opt<std::string, /*ExternalStorage=*/true> remarksAll(
         "remarks-filter",
         cl::desc("Show all remarks: passed, missed, failed, analysis"),
@@ -518,17 +530,23 @@ performActions(raw_ostream &os,
 
   context->enableMultithreading(wasThreadingEnabled);
 
+  // Set the remark categories and policy.
   remark::RemarkCategories cats{
       config.getRemarksAllFilter(), config.getRemarksPassedFilter(),
       config.getRemarksMissedFilter(), config.getRemarksAnalyseFilter(),
       config.getRemarksFailedFilter()};
+  remark::RemarkPolicy policy =
+      config.getRemarkPolicy() == RemarkPolicy::REMARK_POLICY_FINAL
+          ? remark::RemarkPolicy::RemarkPolicyFinal
+          : remark::RemarkPolicy::RemarkPolicyAll;
+  remark::RemarkEngineOpts opts{cats, policy};
 
   mlir::MLIRContext &ctx = *context;
 
   switch (config.getRemarkFormat()) {
   case RemarkFormat::REMARK_FORMAT_STDOUT:
     if (failed(mlir::remark::enableOptimizationRemarks(
-            ctx, nullptr, cats, true /*printAsEmitRemarks*/)))
+            ctx, nullptr, opts, true /*printAsEmitRemarks*/)))
       return failure();
     break;
 
@@ -537,7 +555,7 @@ performActions(raw_ostream &os,
                            ? "mlir-remarks.yaml"
                            : config.getRemarksOutputFile();
     if (failed(mlir::remark::enableOptimizationRemarksWithLLVMStreamer(
-            ctx, file, llvm::remarks::Format::YAML, cats)))
+            ctx, file, llvm::remarks::Format::YAML, opts)))
       return failure();
     break;
   }
@@ -547,7 +565,7 @@ performActions(raw_ostream &os,
                            ? "mlir-remarks.bitstream"
                            : config.getRemarksOutputFile();
     if (failed(mlir::remark::enableOptimizationRemarksWithLLVMStreamer(
-            ctx, file, llvm::remarks::Format::Bitstream, cats)))
+            ctx, file, llvm::remarks::Format::Bitstream, opts)))
       return failure();
     break;
   }

mlir/test/Pass/remark-final.mlir

@@ -0,0 +1,16 @@
+// RUN: mlir-opt %s --test-remark --remarks-filter="category.*" --remark-policy=final --remark-format=yaml --remarks-output-file=%t.yaml
+// RUN: FileCheck %s < %t.yaml
+module @foo {
+  "test.op"() : () -> ()
+
+}
+
+// CHECK-NOT: This is a test passed remark (should be dropped)
+
+// CHECK: !Failure
+// CHECK: Remark:          This is a test failed remark
+// CHECK: !Analysis
+// CHECK: Remark:          This is a test analysis remark
+// CHECK: !Passed
+// CHECK: Remark:          This is a test passed remark
+

mlir/test/lib/Pass/TestRemarksPass.cpp

@@ -43,7 +43,12 @@ class TestRemarkPass : public PassWrapper<TestRemarkPass, OperationPass<>> {
           << remark::add("This is a test missed remark")
           << remark::reason("because we are testing the remark pipeline")
           << remark::suggest("try using the remark pipeline feature");
-
+      mlir::remark::passed(
+          loc,
+          remark::RemarkOpts::name("test-remark").category("category-1-passed"))
+          << remark::add("This is a test passed remark (should be dropped)")
+          << remark::reason("because we are testing the remark pipeline")
+          << remark::suggest("try using the remark pipeline feature");
       mlir::remark::passed(
           loc,
           remark::RemarkOpts::name("test-remark").category("category-1-passed"))