From 4ab7d33359b5dde83fcbfcf63bb3e9a742c1d978 Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Mon, 8 Sep 2025 13:58:13 +0200 Subject: [PATCH 01/15] [MLIR] Add option to postpone remark emission 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. --- mlir/include/mlir/IR/Remarks.h | 27 +++++++--- mlir/lib/IR/Remarks.cpp | 16 +++++- mlir/unittests/IR/RemarkTest.cpp | 87 ++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 7 deletions(-) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index 20e84ec83cd01..94fce3adb32cf 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 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 postponedRemarks; /// Regex that filters missed optimization remarks: only matching one are /// reported. std::optional 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 a55f61aff77bb..04799812e544c 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 5bfca255c22ca..e842d4e1823d7 100644 --- a/mlir/unittests/IR/RemarkTest.cpp +++ b/mlir/unittests/IR/RemarkTest.cpp @@ -315,4 +315,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(), 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 From e6a25f546635398330d24dd0c2705762f68b906c Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Mon, 8 Sep 2025 15:25:19 +0200 Subject: [PATCH 02/15] fx --- mlir/include/mlir/IR/Remarks.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index 94fce3adb32cf..f49ccd8262eeb 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -78,7 +78,7 @@ struct RemarkOpts { constexpr RemarkOpts function(StringRef v) const { return {remarkName, categoryName, subCategoryName, v, postponed}; } - /// Return a copy with the function name set. + /// Return a copy with the postponed flag set. constexpr RemarkOpts postpone() const { return {remarkName, categoryName, subCategoryName, functionName, true}; } @@ -103,7 +103,6 @@ class Remark { .toStringRef(fullCategoryName); } } - ~Remark() = default; // Remark argument that is a key-value pair that can be printed as machine // parsable args. From 34c28a96178760f5f64cc947c6b0767d253bbc66 Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Mon, 8 Sep 2025 16:36:37 +0200 Subject: [PATCH 03/15] fx --- mlir/lib/IR/MLIRContext.cpp | 3 ++ mlir/unittests/IR/RemarkTest.cpp | 88 ++++++++++++++++---------------- 2 files changed, 48 insertions(+), 43 deletions(-) diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp index 1fa04ed8e738f..b9c8bf1de6787 100644 --- a/mlir/lib/IR/MLIRContext.cpp +++ b/mlir/lib/IR/MLIRContext.cpp @@ -278,6 +278,9 @@ class MLIRContextImpl { } } ~MLIRContextImpl() { + // finalize remark engine before destroying anything else. + remarkEngine.reset(); + for (auto typeMapping : registeredTypes) typeMapping.second->~AbstractType(); for (auto attrMapping : registeredAttributes) diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp index e842d4e1823d7..74c941b733be8 100644 --- a/mlir/unittests/IR/RemarkTest.cpp +++ b/mlir/unittests/IR/RemarkTest.cpp @@ -323,13 +323,10 @@ TEST(Remark, TestCustomOptimizationRemarkPostponeDiagnostic) { 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); @@ -344,49 +341,53 @@ TEST(Remark, TestCustomOptimizationRemarkPostponeDiagnostic) { context, std::make_unique(), 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; + // Remark 1: pass, category LoopUnroll + { + remark::passed(loc, remark::RemarkOpts::name("") + .category(categoryLoopunroll) + .subCategory(myPassname2) + .postpone()) + << pass1Msg; + 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"; + } + { + // Postponed remark should not be printed yet. + testing::internal::CaptureStderr(); + // Remark 2: failure, category LoopUnroll + remark::failed(loc, remark::RemarkOpts::name("") + .category(categoryLoopunroll) + .subCategory(myPassname2) + .postpone()) + << remark::reason(pass2Msg); + + 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 be printed) + testing::internal::CaptureStderr(); + remark::passed(loc, remark::RemarkOpts::name("") + .category(categoryLoopunroll) + .subCategory(myPassname1)) + << pass3Msg; + + llvm::errs().flush(); + std::string errOut = testing::internal::GetCapturedStderr(); + auto third = errOut.find("Custom remark:"); + EXPECT_NE(third, std::string::npos); + } 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(); @@ -395,11 +396,12 @@ TEST(Remark, TestCustomOptimizationRemarkPostponeDiagnostic) { EXPECT_NE(first, std::string::npos); auto second = errOut.find("Custom remark:", first + 1); EXPECT_NE(second, std::string::npos); + auto third = errOut.find("Custom remark:", second + 1); + EXPECT_EQ(third, 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 From d4ae63dd9fb8712630b8b4370341b43d7949e01b Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Tue, 16 Sep 2025 09:54:51 +0200 Subject: [PATCH 04/15] fx --- mlir/include/mlir/IR/Remarks.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index f49ccd8262eeb..d6237fdbc2ca5 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -13,6 +13,7 @@ #ifndef MLIR_IR_REMARKS_H #define MLIR_IR_REMARKS_H +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringExtras.h" #include "llvm/IR/DiagnosticInfo.h" #include "llvm/Remarks/Remark.h" @@ -442,6 +443,12 @@ class RemarkEngine { /// Report an analysis remark, this will create an InFlightRemark /// that can be used to build the remark using the << operator. InFlightRemark emitOptimizationRemarkAnalysis(Location loc, RemarkOpts opts); + + /// Get the postponed remarks. + ArrayRef getPostponedRemarks() const { return postponedRemarks; } + + /// Clear the postponed remarks. + void clearPostponedRemarks() { postponedRemarks.clear(); } }; template From d8b77bf94b7a6db1c1cabd50659dff0795d602bc Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Tue, 16 Sep 2025 10:28:29 +0200 Subject: [PATCH 05/15] fx --- mlir/include/mlir/IR/Remarks.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index d6237fdbc2ca5..2b6e4a9110b9b 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -445,10 +445,10 @@ class RemarkEngine { InFlightRemark emitOptimizationRemarkAnalysis(Location loc, RemarkOpts opts); /// Get the postponed remarks. - ArrayRef getPostponedRemarks() const { return postponedRemarks; } + ArrayRef getPostponedRemarks() const { return postponedRemarks; } /// Clear the postponed remarks. - void clearPostponedRemarks() { postponedRemarks.clear(); } + void clearPostponedRemarks() { postponedRemarks.clear(); } }; template From 6c5a3bc5db164af78857c3457ad7eb090bc26fb7 Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Tue, 16 Sep 2025 10:49:18 +0200 Subject: [PATCH 06/15] fx --- mlir/unittests/IR/RemarkTest.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp index 74c941b733be8..ece7b9fb8624a 100644 --- a/mlir/unittests/IR/RemarkTest.cpp +++ b/mlir/unittests/IR/RemarkTest.cpp @@ -280,7 +280,7 @@ TEST(Remark, TestCustomOptimizationRemarkDiagnostic) { Location loc = UnknownLoc::get(&context); // Setup the remark engine - mlir::remark::RemarkCategories cats{/*all=*/"", + mlir::remark::RemarkCategories cats{/*all=*/std::nullopt, /*passed=*/categoryLoopunroll, /*missed=*/std::nullopt, /*analysis=*/std::nullopt, @@ -332,7 +332,8 @@ TEST(Remark, TestCustomOptimizationRemarkPostponeDiagnostic) { Location loc = UnknownLoc::get(&context); // Setup the remark engine - mlir::remark::RemarkCategories cats{/*passed=*/categoryLoopunroll, + mlir::remark::RemarkCategories cats{/*all=*/std::nullopt, + /*passed=*/categoryLoopunroll, /*missed=*/std::nullopt, /*analysis=*/std::nullopt, /*failed=*/categoryLoopunroll}; From 5c6aaa03bd45758baf4137fc8c829fd49ffc6381 Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Wed, 17 Sep 2025 10:51:57 +0200 Subject: [PATCH 07/15] keep public api same --- mlir/include/mlir/IR/Remarks.h | 6 +++++- mlir/lib/IR/Remarks.cpp | 13 +++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index 2b6e4a9110b9b..499b2b11513a3 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -408,6 +408,10 @@ class RemarkEngine { /// Emit all postponed remarks. void emitPostponedRemarks(); + /// Report a remark. When `forcePrintPostponedRemarks` is true, the remark + /// will be printed even if it is postponed. + void report(const Remark &remark, bool forcePrintPostponedRemarks); + public: /// Default constructor is deleted, use the other constructor. RemarkEngine() = delete; @@ -426,7 +430,7 @@ class RemarkEngine { std::string *errMsg); /// Report a remark. - void report(const Remark &remark, bool ignorePostpone = false); + void report(const Remark &remark); /// 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 04799812e544c..9cbe63b16695f 100644 --- a/mlir/lib/IR/Remarks.cpp +++ b/mlir/lib/IR/Remarks.cpp @@ -157,7 +157,7 @@ llvm::remarks::Remark Remark::generateRemark() const { InFlightRemark::~InFlightRemark() { if (remark && owner) - owner->report(std::move(*remark)); + owner->report(*remark); owner = nullptr; } @@ -225,9 +225,10 @@ InFlightRemark RemarkEngine::emitOptimizationRemarkAnalysis(Location loc, // RemarkEngine //===----------------------------------------------------------------------===// -void RemarkEngine::report(const Remark &remark, bool ignorePostpone) { +void RemarkEngine::report(const Remark &remark, + bool forcePrintPostponedRemarks) { // Postponed remarks are shown at the end of pipeline, unless overridden. - if (remark.isPostponed() && !ignorePostpone) { + if (remark.isPostponed() && !forcePrintPostponedRemarks) { postponedRemarks.push_back(remark); return; } @@ -241,9 +242,13 @@ void RemarkEngine::report(const Remark &remark, bool ignorePostpone) { emitRemark(remark.getLocation(), remark.getMsg()); } +void RemarkEngine::report(const Remark &remark) { + report(remark, /*forcePrintPostponedRemarks=*/false); +} + void RemarkEngine::emitPostponedRemarks() { for (auto &remark : postponedRemarks) - report(remark, /*ignorePostpone=*/true); + report(remark, /*forcePrintPostponedRemarks=*/true); postponedRemarks.clear(); } From 2db92d217b96c8cc59da598f891c261b828945d5 Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Wed, 17 Sep 2025 17:46:26 +0200 Subject: [PATCH 08/15] Update mlir/lib/IR/Remarks.cpp Co-authored-by: Mehdi Amini --- mlir/lib/IR/Remarks.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp index 9cbe63b16695f..c11e807684296 100644 --- a/mlir/lib/IR/Remarks.cpp +++ b/mlir/lib/IR/Remarks.cpp @@ -225,14 +225,7 @@ InFlightRemark RemarkEngine::emitOptimizationRemarkAnalysis(Location loc, // RemarkEngine //===----------------------------------------------------------------------===// -void RemarkEngine::report(const Remark &remark, - bool forcePrintPostponedRemarks) { - // Postponed remarks are shown at the end of pipeline, unless overridden. - if (remark.isPostponed() && !forcePrintPostponedRemarks) { - postponedRemarks.push_back(remark); - return; - } - +void RemarkEngine::reportImpl(const Remark &remark) { // Stream the remark if (remarkStreamer) remarkStreamer->streamOptimizationRemark(remark); From 34b80f5a1cabd61d753e782409d91f220f8125cf Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Wed, 17 Sep 2025 17:46:45 +0200 Subject: [PATCH 09/15] Update mlir/include/mlir/IR/Remarks.h Co-authored-by: Mehdi Amini --- mlir/include/mlir/IR/Remarks.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index 499b2b11513a3..ec27d5fc502dc 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -355,7 +355,9 @@ class MLIRRemarkStreamerBase { class RemarkEngine { private: - /// Postponed remarks. They are shown at the end of the pipeline. + /// 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::SmallVector postponedRemarks; /// Regex that filters missed optimization remarks: only matching one are /// reported. From 48666ad9f71e6ad0376f5ec4b5c8c6a083f623f8 Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Wed, 17 Sep 2025 17:46:56 +0200 Subject: [PATCH 10/15] Update mlir/lib/IR/Remarks.cpp Co-authored-by: Mehdi Amini --- mlir/lib/IR/Remarks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp index c11e807684296..8f205431e68fc 100644 --- a/mlir/lib/IR/Remarks.cpp +++ b/mlir/lib/IR/Remarks.cpp @@ -241,7 +241,7 @@ void RemarkEngine::report(const Remark &remark) { void RemarkEngine::emitPostponedRemarks() { for (auto &remark : postponedRemarks) - report(remark, /*forcePrintPostponedRemarks=*/true); + reportImpl(remark); postponedRemarks.clear(); } From 073d604d807a3413885e1e91defc6fc59d525462 Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Wed, 17 Sep 2025 17:47:03 +0200 Subject: [PATCH 11/15] Update mlir/lib/IR/Remarks.cpp Co-authored-by: Mehdi Amini --- mlir/lib/IR/Remarks.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp index 8f205431e68fc..6df002401f05a 100644 --- a/mlir/lib/IR/Remarks.cpp +++ b/mlir/lib/IR/Remarks.cpp @@ -236,7 +236,13 @@ void RemarkEngine::reportImpl(const Remark &remark) { } void RemarkEngine::report(const Remark &remark) { - report(remark, /*forcePrintPostponedRemarks=*/false); + // Postponed remarks are deferred to the end of pipeline. + if (remark.isPostponed()) { + postponedRemarks.push_back(remark); + return; + } + + reportImpl(remark); } void RemarkEngine::emitPostponedRemarks() { From d8f21903d7cec0fd3f50436f371f4d074bc72b0d Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Wed, 17 Sep 2025 17:55:29 +0200 Subject: [PATCH 12/15] fz --- mlir/include/mlir/IR/Remarks.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index ec27d5fc502dc..bfba018d99b82 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -356,8 +356,8 @@ 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. + /// user can intercept them for custom processing, otherwise they will be + /// reported on engine destruction. llvm::SmallVector postponedRemarks; /// Regex that filters missed optimization remarks: only matching one are /// reported. @@ -412,7 +412,7 @@ class RemarkEngine { /// Report a remark. When `forcePrintPostponedRemarks` is true, the remark /// will be printed even if it is postponed. - void report(const Remark &remark, bool forcePrintPostponedRemarks); + void reportImpl(const Remark &remark); public: /// Default constructor is deleted, use the other constructor. From 038769612fc09d88be0b2908e25b29b6f6843e8a Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Thu, 18 Sep 2025 10:09:20 +0200 Subject: [PATCH 13/15] [MLIR][Remark] Add support for dropping remarks This PR adds support for dropping remarks emitted by earlier passes. This is a common use case in optimizing compilers where many passes are involved. For example, consider the following pipeline: ``` pass-early : failed unrolling ... other passes ... pass-late : succeeded unrolling ``` --- mlir/include/mlir/IR/Remarks.h | 92 ++++++++++++++++++++++++-- mlir/lib/IR/Remarks.cpp | 6 +- mlir/unittests/IR/RemarkTest.cpp | 110 ++++++++++++++++++++++++++++++- 3 files changed, 196 insertions(+), 12 deletions(-) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index bfba018d99b82..06a3066cccaa5 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -13,7 +13,8 @@ #ifndef MLIR_IR_REMARKS_H #define MLIR_IR_REMARKS_H -#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/Hashing.h" #include "llvm/ADT/StringExtras.h" #include "llvm/IR/DiagnosticInfo.h" #include "llvm/Remarks/Remark.h" @@ -22,6 +23,7 @@ #include #include "mlir/IR/Diagnostics.h" +#include "mlir/IR/Location.h" #include "mlir/IR/MLIRContext.h" #include "mlir/IR/Value.h" @@ -101,7 +103,7 @@ class Remark { remarkName(opts.remarkName), postponed(opts.postponed) { if (!categoryName.empty() && !subCategoryName.empty()) { (llvm::Twine(categoryName) + ":" + subCategoryName) - .toStringRef(fullCategoryName); + .toStringRef(combinedCategoryName); } } @@ -150,14 +152,14 @@ 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()) return categoryName; if (categoryName.empty()) return subCategoryName; - return fullCategoryName; + return combinedCategoryName; } StringRef getRemarkName() const { @@ -191,7 +193,7 @@ class Remark { StringRef subCategoryName; /// Combined name for category and sub-category - SmallString<64> fullCategoryName; + SmallString<64> combinedCategoryName; /// Remark identifier StringRef remarkName; @@ -358,7 +360,7 @@ class RemarkEngine { /// 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::SmallVector postponedRemarks; + llvm::DenseSet postponedRemarks; /// Regex that filters missed optimization remarks: only matching one are /// reported. std::optional missFilter; @@ -451,10 +453,15 @@ class RemarkEngine { InFlightRemark emitOptimizationRemarkAnalysis(Location loc, RemarkOpts opts); /// Get the postponed remarks. - ArrayRef getPostponedRemarks() const { return postponedRemarks; } + DenseSet getPostponedRemarks() const { return postponedRemarks; } /// Clear the postponed remarks. void clearPostponedRemarks() { postponedRemarks.clear(); } + + /// Drop a postponed remark. + bool dropPostponedRemark(Remark &remark) { + return postponedRemarks.erase(remark); + } }; template @@ -525,6 +532,21 @@ inline detail::InFlightRemark analysis(Location loc, RemarkOpts opts) { return withEngine(&detail::RemarkEngine::emitOptimizationRemarkAnalysis, loc, opts); } +//===----------------------------------------------------------------------===// +// Utils +//===----------------------------------------------------------------------===// + +/// Drop a postponed remark. +inline bool dropPostponedRemark(Location loc, RemarkKind remarkKind, + RemarkOpts opts) { + MLIRContext *ctx = loc->getContext(); + detail::Remark remark(remarkKind, DiagnosticSeverity::Remark, loc, opts); + + // Drop the remark from the postponed remarks. + if (detail::RemarkEngine *engine = ctx->getRemarkEngine()) + return engine->dropPostponedRemark(remark); + return false; +} //===----------------------------------------------------------------------===// // Setup @@ -544,4 +566,60 @@ LogicalResult enableOptimizationRemarks( } // namespace mlir::remark +//===----------------------------------------------------------------------===// +// DenseMapInfo specialization for Remark +//===----------------------------------------------------------------------===// + +namespace llvm { +template <> +struct DenseMapInfo { + static constexpr StringRef kEmptyKey = ""; + static constexpr StringRef kTombstoneKey = ""; + + /// Create an empty remark + static inline mlir::remark::detail::Remark getEmptyKey() { + mlir::MLIRContext dummyContext; + return mlir::remark::detail::Remark( + mlir::remark::RemarkKind::RemarkUnknown, mlir::DiagnosticSeverity::Note, + mlir::UnknownLoc::get(&dummyContext), + mlir::remark::RemarkOpts::name(kEmptyKey)); + } + + /// Create a dead remark + static inline mlir::remark::detail::Remark getTombstoneKey() { + mlir::MLIRContext dummyContext; + return mlir::remark::detail::Remark( + mlir::remark::RemarkKind::RemarkUnknown, mlir::DiagnosticSeverity::Note, + mlir::UnknownLoc::get(&dummyContext), + 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.getCombinedCategoryName()), + static_cast(remark.getRemarkType())); + } + + 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.getCombinedCategoryName() == rhs.getCombinedCategoryName() && + lhs.getRemarkType() == rhs.getRemarkType(); + } +}; +} // namespace llvm + #endif // MLIR_IR_REMARKS_H diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp index 6df002401f05a..9fc8193873ffd 100644 --- a/mlir/lib/IR/Remarks.cpp +++ b/mlir/lib/IR/Remarks.cpp @@ -70,7 +70,7 @@ static void printArgs(llvm::raw_ostream &os, llvm::ArrayRef 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()) { @@ -238,7 +238,7 @@ void RemarkEngine::reportImpl(const Remark &remark) { void RemarkEngine::report(const Remark &remark) { // Postponed remarks are deferred to the end of pipeline. if (remark.isPostponed()) { - postponedRemarks.push_back(remark); + postponedRemarks.insert(remark); return; } diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp index ece7b9fb8624a..0d8bd2e2937a6 100644 --- a/mlir/unittests/IR/RemarkTest.cpp +++ b/mlir/unittests/IR/RemarkTest.cpp @@ -10,9 +10,7 @@ #include "mlir/IR/MLIRContext.h" #include "mlir/IR/Remarks.h" #include "mlir/Remark/RemarkStreamer.h" -#include "mlir/Support/TypeID.h" #include "llvm/ADT/StringRef.h" -#include "llvm/IR/LLVMRemarkStreamer.h" #include "llvm/Remarks/RemarkFormat.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/LogicalResult.h" @@ -405,4 +403,112 @@ TEST(Remark, TestCustomOptimizationRemarkPostponeDiagnostic) { EXPECT_NE(errOut.find(pass2Msg), std::string::npos); // printed } +TEST(Remark, TestRemarkDrop) { + testing::internal::CaptureStderr(); + const auto *pass1Msg = "I am failed"; + const auto *pass2Msg = "I succeeded"; + std::string categoryLoopunroll("LoopUnroll"); + { + MLIRContext context; + Location loc = FileLineColLoc::get(&context, "test.cpp", 1, 5); + Location locOther = FileLineColLoc::get(&context, "test.cpp", 55, 5); + + // Setup the remark engine + mlir::remark::RemarkCategories cats{/*all=*/categoryLoopunroll, + /*passed=*/categoryLoopunroll, + /*missed=*/categoryLoopunroll, + /*analysis=*/categoryLoopunroll, + /*failed=*/categoryLoopunroll}; + + LogicalResult isEnabled = remark::enableOptimizationRemarks( + context, std::make_unique(), cats, true); + ASSERT_TRUE(succeeded(isEnabled)) << "Failed to enable remark engine"; + + // [Pass-early]: Failed remark is emitted as postponed but not shown + { + remark::failed(loc, remark::RemarkOpts::name("unroller") + .category(categoryLoopunroll) + .postpone()) + << pass1Msg; + + // Ensure no remark has been printed yet. + llvm::errs().flush(); + std::string errOut1 = testing::internal::GetCapturedStderr(); + EXPECT_TRUE(errOut1.empty()) + << "Expected no stderr output before postponed remarks are flushed"; + } + testing::internal::CaptureStderr(); + // [Pass-late]: Drop the failed remark, and emit a passed remark + { + // Step 1. Drop the remark from early-pass + bool dropped = remark::dropPostponedRemark( + loc, remark::RemarkKind::RemarkFailure, + remark::RemarkOpts::name("unroller").category(categoryLoopunroll)); + + EXPECT_TRUE(dropped) << "Expected the remark to be dropped"; + + // Step 2. Print the pass remark + remark::passed( + loc, + remark::RemarkOpts::name("unroller").category(categoryLoopunroll)) + << pass2Msg; + } + } + // done with the compilation + llvm::errs().flush(); + std::string errOut2 = testing::internal::GetCapturedStderr(); + + EXPECT_EQ(errOut2.find(pass1Msg), std::string::npos); + EXPECT_NE(errOut2.find(pass2Msg), std::string::npos); +} + +TEST(Remark, TestRemarkCannotDrop) { + testing::internal::CaptureStderr(); + const auto *pass1Msg = "I am failed"; + const auto *pass2Msg = "I succeeded"; + std::string categoryLoopunroll("LoopUnroll"); + { + MLIRContext context; + Location loc = FileLineColLoc::get(&context, "test.cpp", 1, 5); + Location locOther = FileLineColLoc::get(&context, "test.cpp", 55, 5); + // Setup the remark engine + mlir::remark::RemarkCategories cats{/*all=*/categoryLoopunroll, + /*passed=*/categoryLoopunroll, + /*missed=*/categoryLoopunroll, + /*analysis=*/categoryLoopunroll, + /*failed=*/categoryLoopunroll}; + + LogicalResult isEnabled = remark::enableOptimizationRemarks( + context, std::make_unique(), cats, true); + ASSERT_TRUE(succeeded(isEnabled)) << "Failed to enable remark engine"; + // [Pass-early]: Failed remark is emitted as postponed but not shown + { + remark::failed(locOther, remark::RemarkOpts::name("unroller") + .category(categoryLoopunroll) + .postpone()) + << pass1Msg; + } + // [Pass-late]: Drop the failed remark, and emit a passed remark + { + // Step 1. Drop the remark from early-pass + bool dropped = remark::dropPostponedRemark( + loc, remark::RemarkKind::RemarkFailure, + remark::RemarkOpts::name("unroller").category(categoryLoopunroll)); + + EXPECT_FALSE(dropped) << "Expected the remark should not be dropped it " + "has different location"; + + // Step 2. Print the pass remark + remark::passed( + loc, + remark::RemarkOpts::name("unroller").category(categoryLoopunroll)) + << pass2Msg; + } + } + // done with the compilation + llvm::errs().flush(); + std::string errOut3 = testing::internal::GetCapturedStderr(); + EXPECT_NE(errOut3.find(pass1Msg), std::string::npos); // printed + EXPECT_NE(errOut3.find(pass2Msg), std::string::npos); // printed +} } // namespace From 04a3bcc06d78bb77578eaf034a9c4bd64759ac0b Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Thu, 18 Sep 2025 10:42:27 +0200 Subject: [PATCH 14/15] Update mlir/include/mlir/IR/Remarks.h Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- mlir/include/mlir/IR/Remarks.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index 06a3066cccaa5..2648a1006116e 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -453,7 +453,7 @@ class RemarkEngine { InFlightRemark emitOptimizationRemarkAnalysis(Location loc, RemarkOpts opts); /// Get the postponed remarks. - DenseSet getPostponedRemarks() const { return postponedRemarks; } + const DenseSet &getPostponedRemarks() const { return postponedRemarks; } /// Clear the postponed remarks. void clearPostponedRemarks() { postponedRemarks.clear(); } From 260860886bb9e8486f79ea75f7f2dc878e745218 Mon Sep 17 00:00:00 2001 From: Guray Ozen Date: Thu, 18 Sep 2025 10:42:34 +0200 Subject: [PATCH 15/15] Update mlir/include/mlir/IR/Remarks.h Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- mlir/include/mlir/IR/Remarks.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index 2648a1006116e..c74070122b052 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -576,21 +576,25 @@ struct DenseMapInfo { static constexpr StringRef kEmptyKey = ""; static constexpr StringRef kTombstoneKey = ""; + /// 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() { - mlir::MLIRContext dummyContext; return mlir::remark::detail::Remark( mlir::remark::RemarkKind::RemarkUnknown, mlir::DiagnosticSeverity::Note, - mlir::UnknownLoc::get(&dummyContext), + mlir::UnknownLoc::get(getStaticDummyContext()), mlir::remark::RemarkOpts::name(kEmptyKey)); } /// Create a dead remark static inline mlir::remark::detail::Remark getTombstoneKey() { - mlir::MLIRContext dummyContext; return mlir::remark::detail::Remark( mlir::remark::RemarkKind::RemarkUnknown, mlir::DiagnosticSeverity::Note, - mlir::UnknownLoc::get(&dummyContext), + mlir::UnknownLoc::get(getStaticDummyContext()), mlir::remark::RemarkOpts::name(kTombstoneKey)); }