From 0c36a1569a066e2a5fcbaa2835c7b3ea49f60458 Mon Sep 17 00:00:00 2001 From: rkayaith Date: Tue, 11 Oct 2022 15:23:48 -0400 Subject: [PATCH] [mlir][Pass] Disallow mixing -pass-pipeline with other pass options Currently `-pass-pipeline` can be specified multiple times and mixed with the individual `-pass-name` options. Removing this feature will allow for including the pipeline anchor as part of the option argument (see D134900). Reviewed By: rriddle Differential Revision: https://reviews.llvm.org/D135745 --- mlir/include/mlir/Pass/PassRegistry.h | 5 ++- mlir/lib/Pass/PassRegistry.cpp | 68 ++++++++++++++++------------------- mlir/test/Pass/pipeline-parsing.mlir | 3 ++ 3 files changed, 38 insertions(+), 38 deletions(-) diff --git a/mlir/include/mlir/Pass/PassRegistry.h b/mlir/include/mlir/Pass/PassRegistry.h index 4f261e5..9769226 100644 --- a/mlir/include/mlir/Pass/PassRegistry.h +++ b/mlir/include/mlir/Pass/PassRegistry.h @@ -231,7 +231,8 @@ struct PassPipelineCLParserImpl; /// options for each of the passes and pipelines that have been registered with /// the pass registry; Meaning that `-cse` will refer to the CSE pass in MLIR. /// It also registers an argument, `pass-pipeline`, that supports parsing a -/// textual description of a pipeline. +/// textual description of a pipeline. This option is mutually exclusive with +/// the individual pass options. class PassPipelineCLParser { public: /// Construct a pass pipeline parser with the given command line description. @@ -254,6 +255,8 @@ public: private: std::unique_ptr impl; + + llvm::cl::opt passPipeline; }; /// This class implements a command-line parser specifically for MLIR pass diff --git a/mlir/lib/Pass/PassRegistry.cpp b/mlir/lib/Pass/PassRegistry.cpp index 423c97c..31b4115 100644 --- a/mlir/lib/Pass/PassRegistry.cpp +++ b/mlir/lib/Pass/PassRegistry.cpp @@ -12,6 +12,7 @@ #include "mlir/Pass/PassManager.h" #include "mlir/Pass/PassRegistry.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/Support/Format.h" #include "llvm/Support/ManagedStatic.h" #include "llvm/Support/MemoryBuffer.h" @@ -532,6 +533,13 @@ LogicalResult TextualPipeline::initialize(StringRef text, LogicalResult TextualPipeline::addToPipeline( OpPassManager &pm, function_ref errorHandler) const { + // Temporarily disable implicit nesting while we append to the pipeline. We + // want the created pipeline to exactly match the parsed text pipeline, so + // it's preferrable to just error out if implicit nesting would be required. + OpPassManager::Nesting nesting = pm.getNesting(); + pm.setNesting(OpPassManager::Nesting::Explicit); + auto restore = llvm::make_scope_exit([&]() { pm.setNesting(nesting); }); + return addToPipeline(pipeline, pm, errorHandler); } @@ -730,10 +738,6 @@ struct PassArgData { /// This field is set when instance specific pass options have been provided /// on the command line. StringRef options; - - /// This field is used when the parsed option corresponds to an explicit - /// pipeline. - TextualPipeline pipeline; }; } // namespace @@ -775,9 +779,8 @@ struct PassNameParser : public llvm::cl::parser { PassArgData &value); /// If true, this parser only parses entries that correspond to a concrete - /// pass registry entry, and does not add a `pass-pipeline` argument, does not - /// include the options for pass entries, and does not include pass pipelines - /// entries. + /// pass registry entry, and does not include pipeline entries or the options + /// for pass entries. bool passNamesOnly = false; }; } // namespace @@ -785,12 +788,6 @@ struct PassNameParser : public llvm::cl::parser { void PassNameParser::initialize() { llvm::cl::parser::initialize(); - /// Add an entry for the textual pass pipeline option. - if (!passNamesOnly) { - addLiteralOption(passPipelineArg, PassArgData(), - "A textual description of a pass pipeline to run"); - } - /// Add the pass entries. for (const auto &kv : *passRegistry) { addLiteralOption(kv.second.getPassArgument(), &kv.second, @@ -823,11 +820,6 @@ void PassNameParser::printOptionInfo(const llvm::cl::Option &opt, llvm::outs() << " " << opt.HelpStr << '\n'; } - // Print the top-level pipeline argument. - printOptionHelp(passPipelineArg, - "A textual description of a pass pipeline to run", - /*indent=*/4, globalWidth, /*isTopLevel=*/!opt.hasArgStr()); - // Functor used to print the ordered entries of a registration map. auto printOrderedEntries = [&](StringRef header, auto &map) { llvm::SmallVector orderedEntries; @@ -865,11 +857,6 @@ size_t PassNameParser::getOptionWidth(const llvm::cl::Option &opt) const { bool PassNameParser::parse(llvm::cl::Option &opt, StringRef argName, StringRef arg, PassArgData &value) { - // Handle the pipeline option explicitly. - if (argName == passPipelineArg) - return failed(value.pipeline.initialize(arg, llvm::errs())); - - // Otherwise, default to the base for handling. if (llvm::cl::parser::parse(opt, argName, arg, value)) return true; value.options = arg; @@ -907,12 +894,16 @@ struct PassPipelineCLParserImpl { /// Construct a pass pipeline parser with the given command line description. PassPipelineCLParser::PassPipelineCLParser(StringRef arg, StringRef description) : impl(std::make_unique( - arg, description, /*passNamesOnly=*/false)) {} + arg, description, /*passNamesOnly=*/false)), + passPipeline( + StringRef(passPipelineArg), + llvm::cl::desc("Textual description of the pass pipeline to run")) {} PassPipelineCLParser::~PassPipelineCLParser() = default; /// Returns true if this parser contains any valid options to add. bool PassPipelineCLParser::hasAnyOccurrences() const { - return impl->passList.getNumOccurrences() != 0; + return passPipeline.getNumOccurrences() != 0 || + impl->passList.getNumOccurrences() != 0; } /// Returns true if the given pass registry entry was registered at the @@ -925,19 +916,22 @@ bool PassPipelineCLParser::contains(const PassRegistryEntry *entry) const { LogicalResult PassPipelineCLParser::addToPipeline( OpPassManager &pm, function_ref errorHandler) const { + if (passPipeline.getNumOccurrences()) { + if (impl->passList.getNumOccurrences()) + return errorHandler( + "'-" + passPipelineArg + + "' option can't be used with individual pass options"); + std::string errMsg; + llvm::raw_string_ostream os(errMsg); + if (failed(parsePassPipeline(passPipeline, pm, os))) + return errorHandler(errMsg); + return success(); + } + for (auto &passIt : impl->passList) { - if (passIt.registryEntry) { - if (failed(passIt.registryEntry->addToPipeline(pm, passIt.options, - errorHandler))) - return failure(); - } else { - OpPassManager::Nesting nesting = pm.getNesting(); - pm.setNesting(OpPassManager::Nesting::Explicit); - LogicalResult status = passIt.pipeline.addToPipeline(pm, errorHandler); - pm.setNesting(nesting); - if (failed(status)) - return failure(); - } + if (failed(passIt.registryEntry->addToPipeline(pm, passIt.options, + errorHandler))) + return failure(); } return success(); } diff --git a/mlir/test/Pass/pipeline-parsing.mlir b/mlir/test/Pass/pipeline-parsing.mlir index 0ca4897..3e7ce7c 100644 --- a/mlir/test/Pass/pipeline-parsing.mlir +++ b/mlir/test/Pass/pipeline-parsing.mlir @@ -13,6 +13,9 @@ // CHECK_ERROR_4: does not refer to a registered pass or pass pipeline // CHECK_ERROR_5: Can't add pass '{{.*}}TestModulePass' restricted to 'builtin.module' on a PassManager intended to run on 'func.func', did you intend to nest? +// RUN: not mlir-opt %s -pass-pipeline='' -cse 2>&1 | FileCheck --check-prefix=CHECK_ERROR_6 %s +// CHECK_ERROR_6: '-pass-pipeline' option can't be used with individual pass options + func.func @foo() { return } -- 2.7.4