diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index 20e84ec83cd01..dadadd246385b 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -32,6 +32,20 @@ struct RemarkCategories { std::optional 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 postponedRemarks; /// Regex that filters missed optimization remarks: only matching one are /// reported. std::optional missFilter; @@ -357,6 +375,8 @@ class RemarkEngine { std::unique_ptr 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 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 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 { + 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() { + 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 diff --git a/mlir/include/mlir/Remark/RemarkStreamer.h b/mlir/include/mlir/Remark/RemarkStreamer.h index 170d6b439a442..30f39822dcc4c 100644 --- a/mlir/include/mlir/Remark/RemarkStreamer.h +++ b/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 diff --git a/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h b/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h index 0fbe15fa2e0db..b7394387b0f9a 100644 --- a/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h +++ b/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 diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp index 1fa04ed8e738f..358f0fc39b19d 100644 --- a/mlir/lib/IR/MLIRContext.cpp +++ b/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) diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp index a55f61aff77bb..f119d355be59d 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()) { @@ -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 streamer, - const remark::RemarkCategories &cats, bool printAsEmitRemarks) { + const remark::RemarkEngineOpts &opts, bool printAsEmitRemarks) { auto engine = - std::make_unique(printAsEmitRemarks, cats); + std::make_unique(printAsEmitRemarks, opts); std::string errMsg; if (failed(engine->initialize(std::move(streamer), &errMsg))) { diff --git a/mlir/lib/Remark/RemarkStreamer.cpp b/mlir/lib/Remark/RemarkStreamer.cpp index d213a1a2068d6..0737006270293 100644 --- a/mlir/lib/Remark/RemarkStreamer.cpp +++ b/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> 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); } diff --git a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp index 30fd384f3977c..8b0451fdb3555 100644 --- a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp +++ b/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{ + "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 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; } diff --git a/mlir/test/Pass/remark-final.mlir b/mlir/test/Pass/remark-final.mlir new file mode 100644 index 0000000000000..93cde22618bad --- /dev/null +++ b/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 + diff --git a/mlir/test/lib/Pass/TestRemarksPass.cpp b/mlir/test/lib/Pass/TestRemarksPass.cpp index 3b25686b3dc14..5ca2d1a8550aa 100644 --- a/mlir/test/lib/Pass/TestRemarksPass.cpp +++ b/mlir/test/lib/Pass/TestRemarksPass.cpp @@ -43,7 +43,12 @@ class TestRemarkPass : public PassWrapper> { << 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")) diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp index 5bfca255c22ca..9bb07321241ad 100644 --- a/mlir/unittests/IR/RemarkTest.cpp +++ b/mlir/unittests/IR/RemarkTest.cpp @@ -53,10 +53,11 @@ TEST(Remark, TestOutputOptimizationRemark) { /*missed=*/categoryUnroll, /*analysis=*/categoryRegister, /*failed=*/categoryInliner}; - + mlir::remark::RemarkEngineOpts opts{ + cats, mlir::remark::RemarkPolicy::RemarkPolicyAll}; LogicalResult isEnabled = mlir::remark::enableOptimizationRemarksWithLLVMStreamer( - context, yamlFile, llvm::remarks::Format::YAML, cats); + context, yamlFile, llvm::remarks::Format::YAML, opts); ASSERT_TRUE(succeeded(isEnabled)) << "Failed to enable remark engine"; // PASS: something succeeded @@ -203,9 +204,10 @@ TEST(Remark, TestOutputOptimizationRemarkDiagnostic) { /*missed=*/categoryUnroll, /*analysis=*/categoryRegister, /*failed=*/categoryUnroll}; - + mlir::remark::RemarkEngineOpts opts{ + cats, mlir::remark::RemarkPolicy::RemarkPolicyAll}; LogicalResult isEnabled = - remark::enableOptimizationRemarks(context, nullptr, cats, true); + remark::enableOptimizationRemarks(context, nullptr, opts, true); ASSERT_TRUE(succeeded(isEnabled)) << "Failed to enable remark engine"; @@ -285,9 +287,10 @@ TEST(Remark, TestCustomOptimizationRemarkDiagnostic) { /*missed=*/std::nullopt, /*analysis=*/std::nullopt, /*failed=*/categoryLoopunroll}; - + mlir::remark::RemarkEngineOpts opts{ + cats, mlir::remark::RemarkPolicy::RemarkPolicyAll}; LogicalResult isEnabled = remark::enableOptimizationRemarks( - context, std::make_unique(), cats, true); + context, std::make_unique(), opts, true); ASSERT_TRUE(succeeded(isEnabled)) << "Failed to enable remark engine"; // Remark 1: pass, category LoopUnroll @@ -315,4 +318,71 @@ TEST(Remark, TestCustomOptimizationRemarkDiagnostic) { EXPECT_NE(errOut.find(pass2Msg), std::string::npos); // printed EXPECT_EQ(errOut.find(pass3Msg), std::string::npos); // filtered out } + +TEST(Remark, TestRemarkFinal) { + testing::internal::CaptureStderr(); + const auto *pass1Msg = "I failed"; + const auto *pass2Msg = "I failed too"; + const auto *pass3Msg = "I succeeded"; + const auto *pass4Msg = "I succeeded too"; + + std::string categoryLoopunroll("LoopUnroll"); + std::string myPassname1("myPass1"); + std::string myPassname2("myPass2"); + std::string myPassname3("myPass3"); + std::string funcName("myFunc"); + + std::string seenMsg = ""; + + { + 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=*/"", + /*passed=*/categoryLoopunroll, + /*missed=*/categoryLoopunroll, + /*analysis=*/categoryLoopunroll, + /*failed=*/categoryLoopunroll}; + mlir::remark::RemarkEngineOpts opts{ + cats, mlir::remark::RemarkPolicy::RemarkPolicyFinal}; + LogicalResult isEnabled = remark::enableOptimizationRemarks( + context, std::make_unique(), opts, true); + ASSERT_TRUE(succeeded(isEnabled)) << "Failed to enable remark engine"; + + // Remark 1: failure + remark::failed(loc, remark::RemarkOpts::name("Unroller") + .category(categoryLoopunroll) + .subCategory(myPassname1)) + << pass1Msg; + + // Remark 2: failure + remark::missed(loc, remark::RemarkOpts::name("Unroller") + .category(categoryLoopunroll) + .subCategory(myPassname2)) + << remark::reason(pass2Msg); + + // Remark 3: pass + remark::passed(loc, remark::RemarkOpts::name("Unroller") + .category(categoryLoopunroll) + .subCategory(myPassname3)) + << pass3Msg; + + // Remark 4: pass + remark::passed(locOther, remark::RemarkOpts::name("Unroller") + .category(categoryLoopunroll) + .subCategory(myPassname3)) + << pass4Msg; + } + + llvm::errs().flush(); + std::string errOut = ::testing::internal::GetCapturedStderr(); + + // Containment checks for messages. + EXPECT_EQ(errOut.find(pass1Msg), std::string::npos); // dropped + EXPECT_EQ(errOut.find(pass2Msg), std::string::npos); // dropped + EXPECT_NE(errOut.find(pass3Msg), std::string::npos); // shown + EXPECT_NE(errOut.find(pass4Msg), std::string::npos); // shown +} } // namespace