From 1079fc4f543c42bb09a33d2d79d90edd9c0bac91 Mon Sep 17 00:00:00 2001 From: Ivan Butygin Date: Tue, 2 Apr 2024 02:43:04 +0300 Subject: [mlir][pass] Add `errorHandler` param to `Pass::initializeOptions` (#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. See https://github.com/llvm/llvm-project/pull/87166#discussion_r1546426763 * Add error handler callback to `Pass::initializeOptions` * 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. --- mlir/include/mlir/Pass/Pass.h | 4 +++- mlir/include/mlir/Pass/PassOptions.h | 3 ++- .../Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp | 8 +++++--- mlir/lib/Pass/Pass.cpp | 12 ++++++++++-- mlir/lib/Pass/PassRegistry.cpp | 7 ++++--- mlir/lib/Transforms/InlinerPass.cpp | 10 +++++++--- mlir/test/Dialect/Transform/test-pass-application.mlir | 1 + 7 files changed, 32 insertions(+), 13 deletions(-) diff --git a/mlir/include/mlir/Pass/Pass.h b/mlir/include/mlir/Pass/Pass.h index 070e0ca..0f50f30 100644 --- a/mlir/include/mlir/Pass/Pass.h +++ b/mlir/include/mlir/Pass/Pass.h @@ -114,7 +114,9 @@ public: /// 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 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 6717a35..3a5e322 100644 --- a/mlir/include/mlir/Pass/PassOptions.h +++ b/mlir/include/mlir/Pass/PassOptions.h @@ -293,7 +293,8 @@ public: /// 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 76dab8e..4cbc3df 100644 --- a/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp +++ b/mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp @@ -272,14 +272,16 @@ public: const spirv::MemorySpaceToStorageClassMap &memorySpaceMap) : memorySpaceMap(memorySpaceMap) {} - LogicalResult initializeOptions(StringRef options) override { - if (failed(Pass::initializeOptions(options))) + LogicalResult initializeOptions( + StringRef options, + function_ref 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 3fb05e5..57a6c20 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 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 b0c3143..f814967 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 errorHandler) { std::unique_ptr pass = allocator(); - LogicalResult result = pass->initializeOptions(options); + LogicalResult result = pass->initializeOptions(options, errorHandler); std::optional pmOpName = pm.getOpName(); std::optional 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() << ": no such option " << key << "\n"; + errorStream << ": 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 9a7d540..43ca5ca 100644 --- a/mlir/lib/Transforms/InlinerPass.cpp +++ b/mlir/lib/Transforms/InlinerPass.cpp @@ -64,7 +64,9 @@ private: /// 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 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 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 7cb5387..460ac39 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 {{: no such option invalid-option}} transform.apply_registered_pass "canonicalize" to %1 {options = "invalid-option=1"} : (!transform.any_op) -> !transform.any_op transform.yield } -- cgit v1.1