[mlir][pass] Add errorHandler param to Pass::initializeOptions
#87289
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-mlir-spirv @llvm/pr-subscribers-mlir Author: Ivan Butygin (Hardcode84) ChangesThere is no good way to report detailed errors from inside
Full diff: https://github.com/llvm/llvm-project/pull/87289.diff 7 Files Affected:
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/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/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp b/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
index 76dab8ee4ac336..4cbc3dfdae223c 100644
--- a/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
+++ b/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
@@ -272,14 +272,16 @@ 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")
memorySpaceMap = spirv::mapMemorySpaceToOpenCLStorageClass;
else if (clientAPI != "vulkan")
- return failure();
+ return errorHandler(llvm::Twine("Invalid clienAPI: ") + clientAPI);
return success();
}
diff --git a/mlir/lib/Pass/Pass.cpp b/mlir/lib/Pass/Pass.cpp
index 3fb05e53866607..57a6c20141d2c1 100644
--- a/mlir/lib/Pass/Pass.cpp
+++ b/mlir/lib/Pass/Pass.cpp
@@ -60,8 +60,16 @@ Operation *PassExecutionAction::getOp() const {
void Pass::anchor() {}
/// Attempt to initialize the options of this pass from the given string.
-LogicalResult Pass::initializeOptions(StringRef options) {
- return passOptions.parseFromString(options);
+LogicalResult Pass::initializeOptions(
+ StringRef options,
+ function_ref<LogicalResult(const Twine &)> errorHandler) {
+ 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/lib/Pass/PassRegistry.cpp b/mlir/lib/Pass/PassRegistry.cpp
index b0c314369190a4..f8149673a40939 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();
@@ -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))
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
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
}
|
bjacob
added a commit
to iree-org/iree
that referenced
this pull request
Apr 3, 2024
IREE-side changes to adapt to MLIR changes: 1. `initializeOptions` changes to adapt to llvm/llvm-project#87289 2. `enableFastMathMode` removal: llvm/llvm-project#86578. 3. Bazel changes to adapt to llvm/llvm-project#86819 IREE-side fixes for preexisting bugs revealed by a MLIR change: 1. `mlp_tosa` test fix: the shapes were inconsistent, used to accidentally work, until MLIR started catching it since llvm/llvm-project#85798. See diagnostic in [87396](llvm/llvm-project#87396 (comment)). FYI @MaheshRavishankar. IREE-side fixes accidentally lumped into this: 1. The `iree_copts.cmake` change: It just happens that my bleeding-edge Clang was updated and started diagnosing some code relying on C++20 semantics. Filed #16946 as TODO. --------- Co-authored-by: Scott Todd <scott.todd0@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is no good way to report detailed errors from inside
Pass::initializeOptionsfunction as context may not be available at this point and writing directly tollvm::errs()is not composable.See #87166 (comment)
Pass::initializeOptionsPassOptions::parseFromStringto support custom error stream instead of usingllvm::errs()directly.Pass::initializeOptionsimplementation to propagate error string fromparseFromStringto new error handler.MapMemRefStorageClassPassto report error details using new API.