[Mlir-commits] [mlir] [mlir][pass] Add `errorHandler` param to `Pass::initializeOptions` (PR #87289)
Ivan Butygin
llvmlistbot at llvm.org
Mon Apr 1 15:24:45 PDT 2024
https://github.com/Hardcode84 created https://github.com/llvm/llvm-project/pull/87289
There is no good way to report detailed errors from inside `Pass::initializeOptions` function as, context may not be available at this point and writing directly to `llvm::errs()` is not composable.
* Update `PassOptions::parseFromString` to support custom error stream instead of using `llvm::errs()` directly.
* Update default `Pass::initializeOptions` implementation to propagate error string from `parseFromString` to new error handler.
* Update `MapMemRefStorageClassPass` to report error details using new API.
>From 8e9c8ef44be5b16f820a4987207f8cef3c1d72d5 Mon Sep 17 00:00:00 2001
From: Ivan Butygin <ivan.butygin at gmail.com>
Date: Mon, 1 Apr 2024 23:33:35 +0200
Subject: [PATCH 1/4] Update initializeOptions signature
---
mlir/include/mlir/Pass/Pass.h | 4 +++-
.../MemRefToSPIRV/MapMemRefStorageClassPass.cpp | 6 ++++--
mlir/lib/Pass/Pass.cpp | 4 +++-
mlir/lib/Pass/PassRegistry.cpp | 2 +-
mlir/lib/Transforms/InlinerPass.cpp | 10 +++++++---
5 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/mlir/include/mlir/Pass/Pass.h b/mlir/include/mlir/Pass/Pass.h
index 070e0cad38787c..0f50f3064f1780 100644
--- a/mlir/include/mlir/Pass/Pass.h
+++ b/mlir/include/mlir/Pass/Pass.h
@@ -114,7 +114,9 @@ class Pass {
/// Derived classes may override this method to hook into the point at which
/// options are initialized, but should generally always invoke this base
/// class variant.
- virtual LogicalResult initializeOptions(StringRef options);
+ virtual LogicalResult
+ initializeOptions(StringRef options,
+ function_ref<LogicalResult(const Twine &)> errorHandler);
/// Prints out the pass in the textual representation of pipelines. If this is
/// an adaptor pass, print its pass managers.
diff --git a/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp b/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
index 76dab8ee4ac336..6a87008c065129 100644
--- a/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
+++ b/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
@@ -272,8 +272,10 @@ class MapMemRefStorageClassPass final
const spirv::MemorySpaceToStorageClassMap &memorySpaceMap)
: memorySpaceMap(memorySpaceMap) {}
- LogicalResult initializeOptions(StringRef options) override {
- if (failed(Pass::initializeOptions(options)))
+ LogicalResult initializeOptions(
+ StringRef options,
+ function_ref<LogicalResult(const Twine &)> errorHandler) override {
+ if (failed(Pass::initializeOptions(options, errorHandler)))
return failure();
if (clientAPI == "opencl")
diff --git a/mlir/lib/Pass/Pass.cpp b/mlir/lib/Pass/Pass.cpp
index 3fb05e53866607..266c43e8a06b17 100644
--- a/mlir/lib/Pass/Pass.cpp
+++ b/mlir/lib/Pass/Pass.cpp
@@ -60,7 +60,9 @@ Operation *PassExecutionAction::getOp() const {
void Pass::anchor() {}
/// Attempt to initialize the options of this pass from the given string.
-LogicalResult Pass::initializeOptions(StringRef options) {
+LogicalResult Pass::initializeOptions(
+ StringRef options,
+ function_ref<LogicalResult(const Twine &)> errorHandler) {
return passOptions.parseFromString(options);
}
diff --git a/mlir/lib/Pass/PassRegistry.cpp b/mlir/lib/Pass/PassRegistry.cpp
index b0c314369190a4..7a1dc489da9e0a 100644
--- a/mlir/lib/Pass/PassRegistry.cpp
+++ b/mlir/lib/Pass/PassRegistry.cpp
@@ -40,7 +40,7 @@ buildDefaultRegistryFn(const PassAllocatorFunction &allocator) {
return [=](OpPassManager &pm, StringRef options,
function_ref<LogicalResult(const Twine &)> errorHandler) {
std::unique_ptr<Pass> pass = allocator();
- LogicalResult result = pass->initializeOptions(options);
+ LogicalResult result = pass->initializeOptions(options, errorHandler);
std::optional<StringRef> pmOpName = pm.getOpName();
std::optional<StringRef> passOpName = pass->getOpName();
diff --git a/mlir/lib/Transforms/InlinerPass.cpp b/mlir/lib/Transforms/InlinerPass.cpp
index 9a7d5403a95dc5..43ca5cac8b76f3 100644
--- a/mlir/lib/Transforms/InlinerPass.cpp
+++ b/mlir/lib/Transforms/InlinerPass.cpp
@@ -64,7 +64,9 @@ class InlinerPass : public impl::InlinerBase<InlinerPass> {
/// Derived classes may override this method to hook into the point at which
/// options are initialized, but should generally always invoke this base
/// class variant.
- LogicalResult initializeOptions(StringRef options) override;
+ LogicalResult initializeOptions(
+ StringRef options,
+ function_ref<LogicalResult(const Twine &)> errorHandler) override;
/// Inliner configuration parameters created from the pass options.
InlinerConfig config;
@@ -153,8 +155,10 @@ void InlinerPass::runOnOperation() {
return;
}
-LogicalResult InlinerPass::initializeOptions(StringRef options) {
- if (failed(Pass::initializeOptions(options)))
+LogicalResult InlinerPass::initializeOptions(
+ StringRef options,
+ function_ref<LogicalResult(const Twine &)> errorHandler) {
+ if (failed(Pass::initializeOptions(options, errorHandler)))
return failure();
// Initialize the pipeline builder for operations without the dedicated
>From 16d17cc6efbc957a2ea8787261b6c49b9029837f Mon Sep 17 00:00:00 2001
From: Ivan Butygin <ivan.butygin at gmail.com>
Date: Mon, 1 Apr 2024 23:43:11 +0200
Subject: [PATCH 2/4] report error in MapMemRefStorageClassPass
---
mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp b/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
index 6a87008c065129..4cbc3dfdae223c 100644
--- a/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
+++ b/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
@@ -281,7 +281,7 @@ class MapMemRefStorageClassPass final
if (clientAPI == "opencl")
memorySpaceMap = spirv::mapMemorySpaceToOpenCLStorageClass;
else if (clientAPI != "vulkan")
- return failure();
+ return errorHandler(llvm::Twine("Invalid clienAPI: ") + clientAPI);
return success();
}
>From fb7364760aee5239f50072faa7773b5039fd5716 Mon Sep 17 00:00:00 2001
From: Ivan Butygin <ivan.butygin at gmail.com>
Date: Mon, 1 Apr 2024 23:46:18 +0200
Subject: [PATCH 3/4] change parseFromString signature
---
mlir/include/mlir/Pass/PassOptions.h | 3 ++-
mlir/lib/Pass/PassRegistry.cpp | 5 +++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/mlir/include/mlir/Pass/PassOptions.h b/mlir/include/mlir/Pass/PassOptions.h
index 6717a3585d12a5..3a5e3224133e6f 100644
--- a/mlir/include/mlir/Pass/PassOptions.h
+++ b/mlir/include/mlir/Pass/PassOptions.h
@@ -293,7 +293,8 @@ class PassOptions : protected llvm::cl::SubCommand {
/// Parse options out as key=value pairs that can then be handed off to the
/// `llvm::cl` command line passing infrastructure. Everything is space
/// separated.
- LogicalResult parseFromString(StringRef options);
+ LogicalResult parseFromString(StringRef options,
+ raw_ostream &errorStream = llvm::errs());
/// Print the options held by this struct in a form that can be parsed via
/// 'parseFromString'.
diff --git a/mlir/lib/Pass/PassRegistry.cpp b/mlir/lib/Pass/PassRegistry.cpp
index 7a1dc489da9e0a..f8149673a40939 100644
--- a/mlir/lib/Pass/PassRegistry.cpp
+++ b/mlir/lib/Pass/PassRegistry.cpp
@@ -280,7 +280,8 @@ parseNextArg(StringRef options) {
llvm_unreachable("unexpected control flow in pass option parsing");
}
-LogicalResult detail::PassOptions::parseFromString(StringRef options) {
+LogicalResult detail::PassOptions::parseFromString(StringRef options,
+ raw_ostream &errorStream) {
// NOTE: `options` is modified in place to always refer to the unprocessed
// part of the string.
while (!options.empty()) {
@@ -291,7 +292,7 @@ LogicalResult detail::PassOptions::parseFromString(StringRef options) {
auto it = OptionsMap.find(key);
if (it == OptionsMap.end()) {
- llvm::errs() << "<Pass-Options-Parser>: no such option " << key << "\n";
+ errorStream << "<Pass-Options-Parser>: no such option " << key << "\n";
return failure();
}
if (llvm::cl::ProvidePositionalOption(it->second, value, 0))
>From 9b29590b75dfee422ab3011f460a26352e933efc Mon Sep 17 00:00:00 2001
From: Ivan Butygin <ivan.butygin at gmail.com>
Date: Tue, 2 Apr 2024 00:16:27 +0200
Subject: [PATCH 4/4] Update Pass::initializeOptions
---
mlir/lib/Pass/Pass.cpp | 8 +++++++-
mlir/test/Dialect/Transform/test-pass-application.mlir | 1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/mlir/lib/Pass/Pass.cpp b/mlir/lib/Pass/Pass.cpp
index 266c43e8a06b17..57a6c20141d2c1 100644
--- a/mlir/lib/Pass/Pass.cpp
+++ b/mlir/lib/Pass/Pass.cpp
@@ -63,7 +63,13 @@ void Pass::anchor() {}
LogicalResult Pass::initializeOptions(
StringRef options,
function_ref<LogicalResult(const Twine &)> errorHandler) {
- return passOptions.parseFromString(options);
+ std::string errStr;
+ llvm::raw_string_ostream os(errStr);
+ if (failed(passOptions.parseFromString(options, os))) {
+ os.flush();
+ return errorHandler(errStr);
+ }
+ return success();
}
/// Copy the option values from 'other', which is another instance of this
diff --git a/mlir/test/Dialect/Transform/test-pass-application.mlir b/mlir/test/Dialect/Transform/test-pass-application.mlir
index 7cb5387b937d45..460ac3947f5c82 100644
--- a/mlir/test/Dialect/Transform/test-pass-application.mlir
+++ b/mlir/test/Dialect/Transform/test-pass-application.mlir
@@ -78,6 +78,7 @@ module attributes {transform.with_named_sequence} {
transform.named_sequence @__transform_main(%arg1: !transform.any_op) {
%1 = transform.structured.match ops{["func.func"]} in %arg1 : (!transform.any_op) -> !transform.any_op
// expected-error @below {{failed to add pass or pass pipeline to pipeline: canonicalize}}
+ // expected-error @below {{<Pass-Options-Parser>: no such option invalid-option}}
transform.apply_registered_pass "canonicalize" to %1 {options = "invalid-option=1"} : (!transform.any_op) -> !transform.any_op
transform.yield
}
More information about the Mlir-commits
mailing list