-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[MLIR][diag] Add PassManager option to emit failed pass name #155526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Colin De Vlieghere (Cubevoid) ChangesThis patch adds an option to Full diff: https://github.com/llvm/llvm-project/pull/155526.diff 6 Files Affected:
diff --git a/mlir/include/mlir/Pass/PassManager.h b/mlir/include/mlir/Pass/PassManager.h
index 6e59b0f32ac6f..a6021f1e6fd87 100644
--- a/mlir/include/mlir/Pass/PassManager.h
+++ b/mlir/include/mlir/Pass/PassManager.h
@@ -275,6 +275,9 @@ class PassManager : public OpPassManager {
/// Runs the verifier after each individual pass.
void enableVerifier(bool enabled = true);
+ /// Sets whether an error containing the failing pass name should be emitted upon failure.
+ void enableErrorOnFailure(bool enabled = true);
+
//===--------------------------------------------------------------------===//
// Instrumentations
//===--------------------------------------------------------------------===//
@@ -497,6 +500,10 @@ class PassManager : public OpPassManager {
/// A flag that indicates if the IR should be verified in between passes.
bool verifyPasses : 1;
+
+ /// A flag that indicates if an error containing the pass name should be
+ /// emitted upon failure.
+ bool emitErrorOnFailure : 1;
};
/// Register a set of useful command-line options that can be used to configure
diff --git a/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h b/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h
index 94231227599c9..67d6bf98bccea 100644
--- a/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h
+++ b/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h
@@ -207,6 +207,13 @@ class MlirOptMainConfig {
}
bool shouldVerifyPasses() const { return verifyPassesFlag; }
+ /// Set whether to emit and error upon pass failure.
+ MlirOptMainConfig &emitErrorOnPassFailure(bool emit) {
+ emitErrorOnPassFailureFlag = emit;
+ return *this;
+ }
+ bool shouldEmitErrorOnPassFailure() const { return emitErrorOnPassFailureFlag; }
+
/// Set whether to run the verifier on parsing.
MlirOptMainConfig &verifyOnParsing(bool verify) {
disableVerifierOnParsingFlag = !verify;
@@ -290,6 +297,9 @@ class MlirOptMainConfig {
/// Run the verifier after each transformation pass.
bool verifyPassesFlag = true;
+
+ /// Emit an error upon a pass failure.
+ bool emitErrorOnPassFailureFlag = false;
/// Disable the verifier on parsing.
bool disableVerifierOnParsingFlag = false;
diff --git a/mlir/lib/Pass/Pass.cpp b/mlir/lib/Pass/Pass.cpp
index 7094c8e279f2d..ff068083e1f33 100644
--- a/mlir/lib/Pass/Pass.cpp
+++ b/mlir/lib/Pass/Pass.cpp
@@ -498,6 +498,7 @@ llvm::hash_code OpPassManager::hash() {
LogicalResult OpToOpPassAdaptor::run(Pass *pass, Operation *op,
AnalysisManager am, bool verifyPasses,
+ bool emitErrorOnFailure,
unsigned parentInitGeneration) {
std::optional<RegisteredOperationName> opInfo = op->getRegisteredInfo();
if (!opInfo)
@@ -533,9 +534,9 @@ LogicalResult OpToOpPassAdaptor::run(Pass *pass, Operation *op,
if (failed(pipeline.initialize(root->getContext(), parentInitGeneration)))
return failure();
AnalysisManager nestedAm = root == op ? am : am.nest(root);
- return OpToOpPassAdaptor::runPipeline(pipeline, root, nestedAm,
- verifyPasses, parentInitGeneration,
- pi, &parentInfo);
+ return OpToOpPassAdaptor::runPipeline(
+ pipeline, root, nestedAm, verifyPasses, emitErrorOnFailure,
+ parentInitGeneration, pi, &parentInfo);
};
pass->passState.emplace(op, am, dynamicPipelineCallback);
@@ -548,7 +549,7 @@ LogicalResult OpToOpPassAdaptor::run(Pass *pass, Operation *op,
[&]() {
// Invoke the virtual runOnOperation method.
if (auto *adaptor = dyn_cast<OpToOpPassAdaptor>(pass))
- adaptor->runOnOperation(verifyPasses);
+ adaptor->runOnOperation(verifyPasses, emitErrorOnFailure);
else
pass->runOnOperation();
passFailed = pass->passState->irAndPassFailed.getInt();
@@ -597,7 +598,8 @@ LogicalResult OpToOpPassAdaptor::run(Pass *pass, Operation *op,
/// Run the given operation and analysis manager on a provided op pass manager.
LogicalResult OpToOpPassAdaptor::runPipeline(
OpPassManager &pm, Operation *op, AnalysisManager am, bool verifyPasses,
- unsigned parentInitGeneration, PassInstrumentor *instrumentor,
+ bool emitErrorOnFailure, unsigned parentInitGeneration,
+ PassInstrumentor *instrumentor,
const PassInstrumentation::PipelineParentInfo *parentInfo) {
assert((!instrumentor || parentInfo) &&
"expected parent info if instrumentor is provided");
@@ -616,8 +618,12 @@ LogicalResult OpToOpPassAdaptor::runPipeline(
}
for (Pass &pass : pm.getPasses())
- if (failed(run(&pass, op, am, verifyPasses, parentInitGeneration)))
+ if (failed(run(&pass, op, am, verifyPasses, emitErrorOnFailure,
+ parentInitGeneration))) {
+ if (emitErrorOnFailure)
+ return op->emitError("failed to run pass: ") << pass.getName();
return failure();
+ }
if (instrumentor) {
instrumentor->runAfterPipeline(pm.getOpName(*op->getContext()),
@@ -735,15 +741,17 @@ void OpToOpPassAdaptor::runOnOperation() {
}
/// Run the held pipeline over all nested operations.
-void OpToOpPassAdaptor::runOnOperation(bool verifyPasses) {
+void OpToOpPassAdaptor::runOnOperation(bool verifyPasses,
+ bool emitErrorOnFailure) {
if (getContext().isMultithreadingEnabled())
- runOnOperationAsyncImpl(verifyPasses);
+ runOnOperationAsyncImpl(verifyPasses, emitErrorOnFailure);
else
- runOnOperationImpl(verifyPasses);
+ runOnOperationImpl(verifyPasses, emitErrorOnFailure);
}
/// Run this pass adaptor synchronously.
-void OpToOpPassAdaptor::runOnOperationImpl(bool verifyPasses) {
+void OpToOpPassAdaptor::runOnOperationImpl(bool verifyPasses,
+ bool emitErrorOnFailure) {
auto am = getAnalysisManager();
PassInstrumentation::PipelineParentInfo parentInfo = {llvm::get_threadid(),
this};
@@ -758,7 +766,8 @@ void OpToOpPassAdaptor::runOnOperationImpl(bool verifyPasses) {
// Run the held pipeline over the current operation.
unsigned initGeneration = mgr->impl->initializationGeneration;
if (failed(runPipeline(*mgr, &op, am.nest(&op), verifyPasses,
- initGeneration, instrumentor, &parentInfo)))
+ emitErrorOnFailure, initGeneration, instrumentor,
+ &parentInfo)))
signalPassFailure();
}
}
@@ -775,7 +784,8 @@ static bool hasSizeMismatch(ArrayRef<OpPassManager> lhs,
}
/// Run this pass adaptor synchronously.
-void OpToOpPassAdaptor::runOnOperationAsyncImpl(bool verifyPasses) {
+void OpToOpPassAdaptor::runOnOperationAsyncImpl(bool verifyPasses,
+ bool emitErrorOnFailure) {
AnalysisManager am = getAnalysisManager();
MLIRContext *context = &getContext();
@@ -838,7 +848,7 @@ void OpToOpPassAdaptor::runOnOperationAsyncImpl(bool verifyPasses) {
// Get the pass manager for this operation and execute it.
OpPassManager &pm = asyncExecutors[pmIndex][opInfo.passManagerIdx];
LogicalResult pipelineResult = runPipeline(
- pm, opInfo.op, opInfo.am, verifyPasses,
+ pm, opInfo.op, opInfo.am, verifyPasses, emitErrorOnFailure,
pm.impl->initializationGeneration, instrumentor, &parentInfo);
if (failed(pipelineResult))
hasFailure.store(true);
@@ -859,17 +869,21 @@ void OpToOpPassAdaptor::runOnOperationAsyncImpl(bool verifyPasses) {
PassManager::PassManager(MLIRContext *ctx, StringRef operationName,
Nesting nesting)
: OpPassManager(operationName, nesting), context(ctx), passTiming(false),
- verifyPasses(true) {}
+ verifyPasses(true), emitErrorOnFailure(false) {}
PassManager::PassManager(OperationName operationName, Nesting nesting)
: OpPassManager(operationName, nesting),
context(operationName.getContext()), passTiming(false),
- verifyPasses(true) {}
+ verifyPasses(true), emitErrorOnFailure(false) {}
PassManager::~PassManager() = default;
void PassManager::enableVerifier(bool enabled) { verifyPasses = enabled; }
+void PassManager::enableErrorOnFailure(bool enabled) {
+ emitErrorOnFailure = enabled;
+}
+
/// Run the passes within this manager on the provided operation.
LogicalResult PassManager::run(Operation *op) {
MLIRContext *context = getContext();
@@ -931,6 +945,7 @@ void PassManager::addInstrumentation(std::unique_ptr<PassInstrumentation> pi) {
LogicalResult PassManager::runPasses(Operation *op, AnalysisManager am) {
return OpToOpPassAdaptor::runPipeline(*this, op, am, verifyPasses,
+ emitErrorOnFailure,
impl->initializationGeneration);
}
diff --git a/mlir/lib/Pass/PassDetail.h b/mlir/lib/Pass/PassDetail.h
index 5cc726295c9f1..6830debf707ca 100644
--- a/mlir/lib/Pass/PassDetail.h
+++ b/mlir/lib/Pass/PassDetail.h
@@ -29,7 +29,7 @@ class OpToOpPassAdaptor
OpToOpPassAdaptor(const OpToOpPassAdaptor &rhs) = default;
/// Run the held pipeline over all operations.
- void runOnOperation(bool verifyPasses);
+ void runOnOperation(bool verifyPasses, bool emitErrorOnFailure);
void runOnOperation() override;
/// Try to merge the current pass adaptor into 'rhs'. This will try to append
@@ -60,25 +60,29 @@ class OpToOpPassAdaptor
private:
/// Run this pass adaptor synchronously.
- void runOnOperationImpl(bool verifyPasses);
+ void runOnOperationImpl(bool verifyPasses, bool emitErrorOnFailure);
/// Run this pass adaptor asynchronously.
- void runOnOperationAsyncImpl(bool verifyPasses);
+ void runOnOperationAsyncImpl(bool verifyPasses, bool emitErrorOnFailure);
/// Run the given operation and analysis manager on a single pass.
/// `parentInitGeneration` is the initialization generation of the parent pass
/// manager, and is used to initialize any dynamic pass pipelines run by the
- /// given pass.
+ /// given pass. If `emitErrorOnFailure` is set, when a pass in
+ /// the pipeline fails its name will be emitted in an error.
static LogicalResult run(Pass *pass, Operation *op, AnalysisManager am,
- bool verifyPasses, unsigned parentInitGeneration);
+ bool verifyPasses, bool emitErrorOnFailure,
+ unsigned parentInitGeneration);
/// Run the given operation and analysis manager on a provided op pass
/// manager. `parentInitGeneration` is the initialization generation of the
/// parent pass manager, and is used to initialize any dynamic pass pipelines
- /// run by the given passes.
+ /// run by the given passes. If `emitErrorOnFailure` is set, when a pass in
+ /// the pipeline fails its name will be emitted in an error.
static LogicalResult runPipeline(
OpPassManager &pm, Operation *op, AnalysisManager am, bool verifyPasses,
- unsigned parentInitGeneration, PassInstrumentor *instrumentor = nullptr,
+ bool emitErrorOnFailure, unsigned parentInitGeneration,
+ PassInstrumentor *instrumentor = nullptr,
const PassInstrumentation::PipelineParentInfo *parentInfo = nullptr);
/// A set of adaptors to run.
diff --git a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
index de714d8b740af..026b560ac3c46 100644
--- a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
+++ b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
@@ -182,6 +182,11 @@ struct MlirOptMainConfigCLOptions : public MlirOptMainConfig {
cl::desc("Run the verifier after each transformation pass"),
cl::location(verifyPassesFlag), cl::init(true));
+ static cl::opt<bool, /*ExternalStorage=*/true> emitPassErrorOnFailure(
+ "emit-pass-error-on-failure",
+ cl::desc("Emit an error with the pass name when a pass fails"),
+ cl::location(emitErrorOnPassFailureFlag), cl::init(false));
+
static cl::opt<bool, /*ExternalStorage=*/true> disableVerifyOnParsing(
"mlir-very-unsafe-disable-verifier-on-parsing",
cl::desc("Disable the verifier on parsing (very unsafe)"),
@@ -465,6 +470,7 @@ performActions(raw_ostream &os,
// Prepare the pass manager, applying command-line and reproducer options.
PassManager pm(op.get()->getName(), PassManager::Nesting::Implicit);
pm.enableVerifier(config.shouldVerifyPasses());
+ pm.enableErrorOnFailure(config.shouldEmitErrorOnPassFailure());
if (failed(applyPassManagerCLOptions(pm)))
return failure();
pm.enableTiming(timing);
diff --git a/mlir/test/Pass/pass-name-diagnostics.mlir b/mlir/test/Pass/pass-name-diagnostics.mlir
new file mode 100644
index 0000000000000..55e3ebed7426e
--- /dev/null
+++ b/mlir/test/Pass/pass-name-diagnostics.mlir
@@ -0,0 +1,7 @@
+// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(test-pass-failure{gen-diagnostics}))' -emit-pass-error-on-failure -verify-diagnostics=only-expected
+
+// expected-error@+2 {{failed to run pass: (anonymous namespace)::TestFailurePass}}
+// expected-error@+1 {{illegal operation}}
+func.func @TestAlwaysIllegalOperationPass1() {
+ return
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
1a72695
to
8e00a07
Compare
8e00a07
to
0ea3cc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a lot of plumbing for this option, I'm not convinced this is pulling its weight right now.
Passes are supposed to diagnose before "signalPassFailure()", so this error is mostly redundant.
I found some passes that were not emitting a diagnostic before failing, but if the goal is just to debug this, then --mlir-print-ir-after-failure
has always immediately provided the answer to me.
I agree that passes should emit a diagnostic before failing, but in my experience that diagnostic is often too generic to pinpoint on its own without Obviously I don't think this option should be the default, but being able to emit this short error might be more convenient than dumping a potentially huge IR for an end user, IMO. I am open to suggestions though, if there might be a simpler way to communicate this information to a user. There is definitely some nontrivial plumbing right now as you mentioned. |
What about a |
I would prefer if these diagnostics are still available in release mode. Depending on the downstream compiler, the downstream might want to enable something like this by default. |
This patch adds an option to
PassManager
which simply controls whether an error will be emitted when a pass in the pipeline fails. This error includes the name of the pass that failed which could be very helpful for debugging.