[Mlir-commits] [mlir] Revert "[MLIR] Implement remark emitting policies in MLIR" (PR #160681)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Thu Sep 25 03:13:02 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir-core
Author: Mehdi Amini (joker-eph)
<details>
<summary>Changes</summary>
Reverts llvm/llvm-project#<!-- -->160526
This fails with Sanitizers.
---
Patch is 26.38 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/160681.diff
10 Files Affected:
- (modified) mlir/include/mlir/IR/Remarks.h (+6-138)
- (modified) mlir/include/mlir/Remark/RemarkStreamer.h (-1)
- (modified) mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h (-9)
- (modified) mlir/lib/IR/MLIRContext.cpp (-2)
- (modified) mlir/lib/IR/Remarks.cpp (+17-42)
- (modified) mlir/lib/Remark/RemarkStreamer.cpp (+1-3)
- (modified) mlir/lib/Tools/mlir-opt/MlirOptMain.cpp (+4-28)
- (removed) mlir/test/Pass/remark-final.mlir (-12)
- (modified) mlir/test/lib/Pass/TestRemarksPass.cpp (+1-6)
- (modified) mlir/unittests/IR/RemarkTest.cpp (+6-74)
``````````diff
diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 5e2bd828bd00e..20e84ec83cd01 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -24,8 +24,6 @@
#include "mlir/IR/MLIRContext.h"
#include "mlir/IR/Value.h"
-#include <functional>
-
namespace mlir::remark {
/// Define an the set of categories to accept. By default none are, the provided
@@ -146,7 +144,7 @@ class Remark {
llvm::StringRef getCategoryName() const { return categoryName; }
- llvm::StringRef getCombinedCategoryName() const {
+ llvm::StringRef getFullCategoryName() const {
if (categoryName.empty() && subCategoryName.empty())
return {};
if (subCategoryName.empty())
@@ -320,7 +318,7 @@ class InFlightRemark {
};
//===----------------------------------------------------------------------===//
-// Pluggable Remark Utilities
+// MLIR Remark Streamer
//===----------------------------------------------------------------------===//
/// Base class for MLIR remark streamers that is used to stream
@@ -340,26 +338,6 @@ class MLIRRemarkStreamerBase {
virtual void finalize() {} // optional
};
-using ReportFn = llvm::unique_function<void(const Remark &)>;
-
-/// Base class for MLIR remark emitting policies that is used to emit
-/// optimization remarks to the underlying remark streamer. The derived classes
-/// should implement the `reportRemark` method to provide the actual emitting
-/// implementation.
-class RemarkEmittingPolicyBase {
-protected:
- ReportFn reportImpl;
-
-public:
- RemarkEmittingPolicyBase() = default;
- virtual ~RemarkEmittingPolicyBase() = default;
-
- void initialize(ReportFn fn) { reportImpl = std::move(fn); }
-
- virtual void reportRemark(const Remark &remark) = 0;
- virtual void finalize() = 0;
-};
-
//===----------------------------------------------------------------------===//
// Remark Engine (MLIR Context will own this class)
//===----------------------------------------------------------------------===//
@@ -377,8 +355,6 @@ class RemarkEngine {
std::optional<llvm::Regex> failedFilter;
/// The MLIR remark streamer that will be used to emit the remarks.
std::unique_ptr<MLIRRemarkStreamerBase> remarkStreamer;
- /// The MLIR remark policy that will be used to emit the remarks.
- std::unique_ptr<RemarkEmittingPolicyBase> remarkEmittingPolicy;
/// When is enabled, engine also prints remarks as mlir::emitRemarks.
bool printAsEmitRemarks = false;
@@ -416,8 +392,6 @@ class RemarkEngine {
InFlightRemark emitIfEnabled(Location loc, RemarkOpts opts,
bool (RemarkEngine::*isEnabled)(StringRef)
const);
- /// Report a remark.
- void reportImpl(const Remark &remark);
public:
/// Default constructor is deleted, use the other constructor.
@@ -433,10 +407,8 @@ class RemarkEngine {
~RemarkEngine();
/// Setup the remark engine with the given output path and format.
- LogicalResult
- initialize(std::unique_ptr<MLIRRemarkStreamerBase> streamer,
- std::unique_ptr<RemarkEmittingPolicyBase> remarkEmittingPolicy,
- std::string *errMsg);
+ LogicalResult initialize(std::unique_ptr<MLIRRemarkStreamerBase> streamer,
+ std::string *errMsg);
/// Report a remark.
void report(const Remark &&remark);
@@ -474,54 +446,6 @@ inline InFlightRemark withEngine(Fn fn, Location loc, Args &&...args) {
namespace mlir::remark {
-//===----------------------------------------------------------------------===//
-// Remark Emitting Policies
-//===----------------------------------------------------------------------===//
-
-/// Policy that emits all remarks.
-class RemarkEmittingPolicyAll : public detail::RemarkEmittingPolicyBase {
-public:
- RemarkEmittingPolicyAll();
-
- void reportRemark(const detail::Remark &remark) override {
- reportImpl(remark);
- }
- void finalize() override {}
-};
-
-/// Policy that emits final remarks.
-class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
-private:
- /// user can intercept them for custom processing via a registered callback,
- /// otherwise they will be reported on engine destruction.
- llvm::DenseSet<detail::Remark> postponedRemarks;
- /// Optional user callback for intercepting postponed remarks.
- std::function<void(const detail::Remark &)> postponedRemarkCallback;
-
-public:
- RemarkEmittingPolicyFinal();
-
- /// Register a callback to intercept postponed remarks before they are
- /// reported. The callback will be invoked for each postponed remark in
- /// finalize().
- void
- setPostponedRemarkCallback(std::function<void(const detail::Remark &)> cb) {
- postponedRemarkCallback = std::move(cb);
- }
-
- void reportRemark(const detail::Remark &remark) override {
- postponedRemarks.erase(remark);
- postponedRemarks.insert(remark);
- }
- void finalize() override {
- for (auto &remark : postponedRemarks) {
- if (postponedRemarkCallback)
- postponedRemarkCallback(remark);
- reportImpl(remark);
- }
- }
-};
-
/// Create a Reason with llvm::formatv formatting.
template <class... Ts>
inline detail::LazyTextBuild reason(const char *fmt, Ts &&...ts) {
@@ -581,72 +505,16 @@ inline detail::InFlightRemark analysis(Location loc, RemarkOpts opts) {
/// Setup remarks for the context. This function will enable the remark engine
/// and set the streamer to be used for optimization remarks. The remark
-/// categories are used to filter the remarks that will be emitted by the
-/// remark engine. If a category is not specified, it will not be emitted. If
+/// categories are used to filter the remarks that will be emitted by the remark
+/// engine. If a category is not specified, it will not be emitted. If
/// `printAsEmitRemarks` is true, the remarks will be printed as
/// mlir::emitRemarks. 'streamer' must inherit from MLIRRemarkStreamerBase and
/// will be used to stream the remarks.
LogicalResult enableOptimizationRemarks(
MLIRContext &ctx,
std::unique_ptr<remark::detail::MLIRRemarkStreamerBase> streamer,
- std::unique_ptr<remark::detail::RemarkEmittingPolicyBase>
- remarkEmittingPolicy,
const remark::RemarkCategories &cats, 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.getCombinedCategoryName()));
- }
-
- 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();
- }
-};
-} // namespace llvm
#endif // MLIR_IR_REMARKS_H
diff --git a/mlir/include/mlir/Remark/RemarkStreamer.h b/mlir/include/mlir/Remark/RemarkStreamer.h
index 19a70fa4c4daa..170d6b439a442 100644
--- a/mlir/include/mlir/Remark/RemarkStreamer.h
+++ b/mlir/include/mlir/Remark/RemarkStreamer.h
@@ -45,7 +45,6 @@ namespace mlir::remark {
/// mlir::emitRemarks.
LogicalResult enableOptimizationRemarksWithLLVMStreamer(
MLIRContext &ctx, StringRef filePath, llvm::remarks::Format fmt,
- std::unique_ptr<detail::RemarkEmittingPolicyBase> remarkEmittingPolicy,
const RemarkCategories &cat, 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 b7394387b0f9a..0fbe15fa2e0db 100644
--- a/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h
+++ b/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h
@@ -44,11 +44,6 @@ 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.
@@ -247,8 +242,6 @@ 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.
@@ -272,8 +265,6 @@ 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 358f0fc39b19d..1fa04ed8e738f 100644
--- a/mlir/lib/IR/MLIRContext.cpp
+++ b/mlir/lib/IR/MLIRContext.cpp
@@ -278,8 +278,6 @@ 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 dca2da67d8a98..a55f61aff77bb 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -16,7 +16,7 @@
#include "llvm/ADT/StringRef.h"
using namespace mlir::remark::detail;
-using namespace mlir::remark;
+
//------------------------------------------------------------------------------
// Remark
//------------------------------------------------------------------------------
@@ -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 = getCombinedCategoryName();
+ StringRef categoryName = getFullCategoryName();
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 = getCombinedCategoryName();
+ r.PassName = getFullCategoryName();
r.FunctionName = getFunction();
r.Loc = locLambda();
for (const Remark::Arg &arg : getArgs()) {
@@ -225,7 +225,7 @@ InFlightRemark RemarkEngine::emitOptimizationRemarkAnalysis(Location loc,
// RemarkEngine
//===----------------------------------------------------------------------===//
-void RemarkEngine::reportImpl(const Remark &remark) {
+void RemarkEngine::report(const Remark &&remark) {
// Stream the remark
if (remarkStreamer)
remarkStreamer->streamOptimizationRemark(remark);
@@ -235,19 +235,19 @@ void RemarkEngine::reportImpl(const Remark &remark) {
emitRemark(remark.getLocation(), remark.getMsg());
}
-void RemarkEngine::report(const Remark &&remark) {
- if (remarkEmittingPolicy)
- remarkEmittingPolicy->reportRemark(remark);
-}
-
RemarkEngine::~RemarkEngine() {
- if (remarkEmittingPolicy)
- remarkEmittingPolicy->finalize();
-
if (remarkStreamer)
remarkStreamer->finalize();
}
+llvm::LogicalResult
+RemarkEngine::initialize(std::unique_ptr<MLIRRemarkStreamerBase> streamer,
+ std::string *errMsg) {
+ // If you need to validate categories/filters, do so here and set errMsg.
+ remarkStreamer = std::move(streamer);
+ return success();
+}
+
/// Returns true if filter is already anchored like ^...$
static bool isAnchored(llvm::StringRef s) {
s = s.trim();
@@ -300,31 +300,15 @@ RemarkEngine::RemarkEngine(bool printAsEmitRemarks,
failedFilter = buildFilter(cats, cats.failed);
}
-llvm::LogicalResult RemarkEngine::initialize(
- std::unique_ptr<MLIRRemarkStreamerBase> streamer,
- std::unique_ptr<RemarkEmittingPolicyBase> remarkEmittingPolicy,
- std::string *errMsg) {
-
- remarkStreamer = std::move(streamer);
-
- // Capture `this`. Ensure RemarkEngine is not moved after this.
- auto reportFunc = [this](const Remark &r) { this->reportImpl(r); };
- remarkEmittingPolicy->initialize(ReportFn(std::move(reportFunc)));
-
- this->remarkEmittingPolicy = std::move(remarkEmittingPolicy);
- return success();
-}
-
llvm::LogicalResult mlir::remark::enableOptimizationRemarks(
- MLIRContext &ctx, std::unique_ptr<detail::MLIRRemarkStreamerBase> streamer,
- std::unique_ptr<detail::RemarkEmittingPolicyBase> remarkEmittingPolicy,
- const RemarkCategories &cats, bool printAsEmitRemarks) {
+ MLIRContext &ctx,
+ std::unique_ptr<remark::detail::MLIRRemarkStreamerBase> streamer,
+ const remark::RemarkCategories &cats, bool printAsEmitRemarks) {
auto engine =
- std::make_unique<detail::RemarkEngine>(printAsEmitRemarks, cats);
+ std::make_unique<remark::detail::RemarkEngine>(printAsEmitRemarks, cats);
std::string errMsg;
- if (failed(engine->initialize(std::move(streamer),
- std::move(remarkEmittingPolicy), &errMsg))) {
+ if (failed(engine->initialize(std::move(streamer), &errMsg))) {
llvm::report_fatal_error(
llvm::Twine("Failed to initialize remark engine. Error: ") + errMsg);
}
@@ -332,12 +316,3 @@ llvm::LogicalResult mlir::remark::enableOptimizationRemarks(
return success();
}
-
-//===----------------------------------------------------------------------===//
-// Remark emitting policies
-//===----------------------------------------------------------------------===//
-
-namespace mlir::remark {
-RemarkEmittingPolicyAll::RemarkEmittingPolicyAll() = default;
-RemarkEmittingPolicyFinal::RemarkEmittingPolicyFinal() = default;
-} // namespace mlir::remark
diff --git a/mlir/lib/Remark/RemarkStreamer.cpp b/mlir/lib/Remark/RemarkStreamer.cpp
index bf362862d24f6..d213a1a2068d6 100644
--- a/mlir/lib/Remark/RemarkStreamer.cpp
+++ b/mlir/lib/Remark/RemarkStreamer.cpp
@@ -60,7 +60,6 @@ void LLVMRemarkStreamer::finalize() {
namespace mlir::remark {
LogicalResult enableOptimizationRemarksWithLLVMStreamer(
MLIRContext &ctx, StringRef path, llvm::remarks::Format fmt,
- std::unique_ptr<detail::RemarkEmittingPolicyBase> remarkEmittingPolicy,
const RemarkCategories &cat, bool printAsEmitRemarks) {
FailureOr<std::unique_ptr<detail::MLIRRemarkStreamerBase>> sOr =
@@ -68,8 +67,7 @@ LogicalResult enableOptimizationRemarksWithLLVMStreamer(
if (failed(sOr))
return failure();
- return remark::enableOptimizationRemarks(ctx, std::move(*sOr),
- std::move(remarkEmittingPolicy), cat,
+ return remark::enableOptimizationRemarks(ctx, std::move(*sOr), cat,
printAsEmitRemarks);
}
diff --git a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
index 212793c22d152..30fd384f3977c 100644
--- a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
+++ b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
@@ -37,7 +37,6 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Remarks/RemarkFormat.h"
#include "llvm/Support/CommandLine.h"
-#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/InitLLVM.h"
#include "llvm/Support/LogicalResult.h"
#include "llvm/Support/ManagedStatic.h"
@@ -46,7 +45,6 @@
#include "llvm/Support/SourceMgr.h"
#include "llvm/Support/ThreadPool.h"
#include "llvm/Support/ToolOutputFile.h"
-#include <memory>
using namespace mlir;
using namespace llvm;
@@ -228,18 +226,6 @@ 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"),
@@ -531,28 +517,18 @@ performActions(raw_ostream &os,
return failure();
context->enableMultithreading(wasThreadingEnabled);
- // Set the remark categories and policy.
+
remark::RemarkCategories cats{
config.getRemarksAllFilter(), config.getRemarksPassedFilter(),
config.getRemarksMissedFilter(), config.getRemarksAnalyseFilter(),
config.getRemarksFailedFilter()};
mlir::MLIRContext &ctx = *context;
- // Helper to create the appropriate policy based on configuration
- auto createPolicy = [&config]()
- -> std::unique_ptr<mlir::remark::detail::RemarkEmittingPolicyBase> {
- if (config.getRemarkPolicy() == RemarkPolicy::REMARK_POLICY_ALL)
- return std::make_unique<mlir::remark::RemarkEmittingPolicyAll>();
- if (config.getRemarkPolicy() == RemarkPolicy::REMARK_POLICY_FINAL)
- return std::make_unique<mlir::remark::RemarkEmittingPolicyFinal>();
-
- llvm_unreachable("Invalid remark policy");
- };
switch (config.getRemarkFormat()) {
case RemarkFormat::REMARK_FORMAT_STDOUT:
if (failed(mlir::remark::enableOptimizationRemarks(
- ctx, nullptr, createPolicy(), cats, true /*printAsEmitRemarks*/)))
+ ctx, nullptr, cats, true /*printAsEmitRemarks*/)))
return failure();
break;
@@ -561,7 +537,7 @@ performActions(raw_ostream &os,
? "mlir-remarks.yaml"
: config.getRemarksOutputFile();
if (failed(mlir::remark::enableOptimizationRemarksWithLLVMStreamer(
- ctx, file, llvm::remarks::Format::YAML, createPolicy(), cats)))
+ ctx, file, llvm::remarks::Format::YAML, cats)))
return failure();
break;
}
@@ -571,7 +547,7 @@ performActions(raw_ostream &os,
? "mlir-remarks.bitstream"
: config.getRemarksOutputFile();
if (failed(mlir::remark::enableOptimizationRemarksWithLLVMStreamer(
- ctx, file, llvm::remarks::Format::Bitstream, createPolicy(), cats)))
+ ctx, file, llvm::remarks::Format::Bitstream, cats)))
...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/160681
More information about the Mlir-commits
mailing list