[mlir-reduce] Improve diagnostic message and clean build dependency

[rriddle]

Looks good, thanks!

mlir/lib/Reducer/ReductionTreePass.cpp

87

nit: diagnostics should start with a lower case.

157–159

?

mlir/lib/Tools/mlir-reduce/MlirReduceMain.cpp

61–62

Should we just register all of the passes? Is there a reason to do just the one?

[Chia-hungDuan]

Addressed review comments, added a OptionCategory for mlir-reduce and hid the unrelated options
Now only mlir passes will be shown.

mlir/include/mlir/Reducer/Passes.td

 #ifndef MLIR_REDUCER_PASSES
 #define MLIR_REDUCER_PASSES
 
 include "mlir/Pass/PassBase.td"
 
 def CommonReductionPassOptions {
   list<Option> options = [
     Option<"testerName", "test", "std::string", /* default */"",
-           "The filename of the tester">,
+           "The location of the tester which tests the file interestingness">,
     ListOption<"testerArgs", "test-arg", "std::string",
+               "arguments of the tester",
                "llvm::cl::ZeroOrMore, llvm::cl::MiscFlags::CommaSeparated">,
   ];
 }
 
 def ReductionTree : Pass<"reduction-tree"> {
-  let summary = "A general reduction tree pass for the MLIR Reduce Tool";
+  let summary = "Reduce the input with reduction-tree algorithm";
 
   let constructor = "mlir::createReductionTreePass()";
 
   let options = [
     Option<"traversalModeId", "traversal-mode", "unsigned",
-           /* default */"0", "The graph traversal mode">,
+           /* default */"0",
+           "The graph traversal mode, the default is single-path mode">,
   ] # CommonReductionPassOptions.options;
 }
 
 def OptReduction : Pass<"opt-reduction-pass", "ModuleOp"> {
-  let summary = "A reduction pass wrapper for optimization passes";
+  let summary = "A wrapper pass that reduces the file with optimization passes";
 
   let constructor = "mlir::createOptReductionPass()";
 
   let options = [
     Option<"optPass", "opt-pass", "std::string", /* default */"",
-           "The optimization pass will be run dynamically in OptReductionPass">,
+           "The optimization passes used for reduction, e.g., symbol-dce">,
   ] # CommonReductionPassOptions.options;
 }
 
 #endif // MLIR_REDUCER_PASSES

mlir/lib/Reducer/OptReductionPass.cpp

void OptReductionPass::runOnOperation() {
 
   Tester test(testerName, testerArgs);
 
   ModuleOp module = this->getOperation();
   ModuleOp moduleVariant = module.clone();
 
   PassManager passManager(module.getContext());
   if (failed(parsePassPipeline(optPass, passManager))) {
-    LLVM_DEBUG(llvm::dbgs() << "\nFailed to parse pass pipeline");
-    return;
+    module.emitError() << "\nfailed to parse pass pipeline";
+    return signalPassFailure();
   }
 
   std::pair<Tester::Interestingness, int> original = test.isInteresting(module);
   if (original.first != Tester::Interestingness::True) {
-    LLVM_DEBUG(llvm::dbgs() << "\nThe original input is not interested");
-    return;
+    module.emitError() << "\nthe original input is not interested";
+    return signalPassFailure();
   }
 
   if (failed(passManager.run(moduleVariant))) {
-    LLVM_DEBUG(llvm::dbgs() << "\nFailed to run pass pipeline");
-    return;
+    module.emitError() << "\nfailed to run pass pipeline";
+    return signalPassFailure();
   }
 
   std::pair<Tester::Interestingness, int> reduced =
       test.isInteresting(moduleVariant);
 
   if (reduced.first == Tester::Interestingness::True &&
       reduced.second < original.second) {
     module.getBody()->clear();

mlir/lib/Reducer/ReductionTreePass.cpp

 /// We will apply the reducer patterns to the operations in the ranges specified
 /// by ReductionNode. Note that we are not able to remove an operation without
 /// replacing it with another valid operation. However, The validity of module
 /// reduction is based on the Tester provided by the user and that means certain
 /// invalid module is still interested by the use. Thus we provide an
 /// alternative way to remove operations, which is using `eraseOpNotInRange` to
 /// erase the operations not in the range specified by ReductionNode.
 template <typename IteratorType>
-static void findOptimal(ModuleOp module, Region &region,
+static LogicalResult findOptimal(ModuleOp module, Region &region,
                                  const FrozenRewritePatternSet &patterns,
                                  const Tester &test, bool eraseOpNotInRange) {
   std::pair<Tester::Interestingness, size_t> initStatus =
       test.isInteresting(module);
   // While exploring the reduction tree, we always branch from an interesting
   // node. Thus the root node must be interesting.
   if (initStatus.first != Tester::Interestingness::True)
-    return;
+    return module.emitWarning() << "uninterested module will not be reduced";
 
   llvm::SpecificBumpPtrAllocator<ReductionNode> allocator;
 
   std::vector<ReductionNode::Range> ranges{
       {0, std::distance(region.op_begin(), region.op_end())}};
 
   ReductionNode *root = allocator.Allocate();
   new (root) ReductionNode(nullptr, std::move(ranges), allocator);
while (!trace.empty()) {
     applyPatterns(region, patterns, top->getStartRanges(), eraseOpNotInRange);
   }
 
   if (test.isInteresting(module).first != Tester::Interestingness::True)
     llvm::report_fatal_error("Reduced module is not interesting");
   if (test.isInteresting(module).second != smallestNode->getSize())
     llvm::report_fatal_error(
         "Reduced module doesn't have consistent size with smallestNode");
+  return success();
 }
 
 template <typename IteratorType>
-static void findOptimal(ModuleOp module, Region &region,
+static LogicalResult findOptimal(ModuleOp module, Region &region,
                                  const FrozenRewritePatternSet &patterns,
                                  const Tester &test) {
   // We separate the reduction process into 2 steps, the first one is to erase
   // redundant operations and the second one is to apply the reducer patterns.
 
   // In the first phase, we don't apply any patterns so that we only select the
   // range of operations to keep to the module stay interesting.
-  findOptimal<IteratorType>(module, region, /*patterns=*/{}, test,
-                            /*eraseOpNotInRange=*/true);
+  if (failed(findOptimal<IteratorType>(module, region, /*patterns=*/{}, test,
+                                       /*eraseOpNotInRange=*/true)))
+    return failure();
   // In the second phase, we suppose that no operation is redundant, so we try
   // to rewrite the operation into simpler form.
-  findOptimal<IteratorType>(module, region, patterns, test,
+  return findOptimal<IteratorType>(module, region, patterns, test,
                                    /*eraseOpNotInRange=*/false);
 }
 
 namespace {
 
 //===----------------------------------------------------------------------===//
 // Reduction Pattern Interface Collection
 //===----------------------------------------------------------------------===//
 
 class ReductionPatternInterfaceCollection
public:
   ReductionTreePass(const ReductionTreePass &pass) = default;
 
   LogicalResult initialize(MLIRContext *context) override;
 
   /// Runs the pass instance in the pass pipeline.
   void runOnOperation() override;
 
 private:
-  void reduceOp(ModuleOp module, Region &region);
+  LogicalResult reduceOp(ModuleOp module, Region &region);
 
   FrozenRewritePatternSet reducerPatterns;
 };
 
 } // end anonymous namespace
 
 LogicalResult ReductionTreePass::initialize(MLIRContext *context) {
   RewritePatternSet patterns(context);
void ReductionTreePass::runOnOperation() {
   SmallVector<Operation *, 8> workList;
   workList.push_back(getOperation());
 
   do {
     Operation *op = workList.pop_back_val();
 
     for (Region &region : op->getRegions())
       if (!region.empty())
-        reduceOp(module, region);
+        if (failed(reduceOp(module, region)))
+          return signalPassFailure();
 
     for (Region &region : op->getRegions())
       for (Operation &op : region.getOps())
         if (op.getNumRegions() != 0)
           workList.push_back(&op);
   } while (!workList.empty());
 }
 
-void ReductionTreePass::reduceOp(ModuleOp module, Region &region) {
+LogicalResult ReductionTreePass::reduceOp(ModuleOp module, Region &region) {
   Tester test(testerName, testerArgs);
   switch (traversalModeId) {
   case TraversalMode::SinglePath:
-    findOptimal<ReductionNode::iterator<TraversalMode::SinglePath>>(
+    return findOptimal<ReductionNode::iterator<TraversalMode::SinglePath>>(
         module, region, reducerPatterns, test);
-    break;
   default:
-    llvm_unreachable("Unsupported mode");
+    return module.emitError() << "unsupported traversal mode detected";
   }
 }
 
 std::unique_ptr<Pass> mlir::createReductionTreePass() {
   return std::make_unique<ReductionTreePass>();
 }

mlir/lib/Tools/mlir-reduce/CMakeLists.txt

-set(LLVM_OPTIONAL_SOURCES
-  MlirReduceMain.cpp
-)
-
-set(LLVM_LINK_COMPONENTS
-  Support
-  )
-
 add_mlir_library(MLIRReduceLib
   MlirReduceMain.cpp
 
   LINK_LIBS PUBLIC
   MLIRIR
   MLIRParser
   MLIRPass
   MLIRReduce
   MLIRSupport
   )

mlir/lib/Tools/mlir-reduce/MlirReduceMain.cpp

static LogicalResult loadModule(MLIRContext &context, OwningModuleRef &module,
   if (!module)
     return failure();
 
   return success();
 }
 
 LogicalResult mlir::mlirReduceMain(int argc, char **argv,
                                    MLIRContext &context) {
+  // Override the default '-h' and use the default PrintHelpMessage() which
+  // won't print options in categories.
+  static llvm::cl::opt<bool> Help("h", llvm::cl::desc("Alias for -help"),
+                                  llvm::cl::Hidden);
+
+  static llvm::cl::OptionCategory MLIRReduceCategory("mlir-reduce options");
+
   static llvm::cl::opt<std::string> inputFilename(
-      llvm::cl::Positional, llvm::cl::Required, llvm::cl::desc("<input file>"));
+      llvm::cl::Positional, llvm::cl::desc("<input file>"),
+      llvm::cl::cat(MLIRReduceCategory));
 
   static llvm::cl::opt<std::string> outputFilename(
       "o", llvm::cl::desc("Output filename for the reduced test case"),
-      llvm::cl::init("-"));
+      llvm::cl::init("-"), llvm::cl::cat(MLIRReduceCategory));
+
+  llvm::cl::HideUnrelatedOptions(MLIRReduceCategory);
 
   llvm::InitLLVM y(argc, argv);
 
   registerReducerPasses();
-  registerMLIRContextCLOptions();
-  registerPassManagerCLOptions();
 
   PassPipelineCLParser parser("", "Reduction Passes to Run");
   llvm::cl::ParseCommandLineOptions(argc, argv,
                                     "MLIR test case reduction tool.\n");
 
+  if (Help) {
+    llvm::cl::PrintHelpMessage();
+    return success();
+  }
+
   std::string errorMessage;
 
   auto output = openOutputFile(outputFilename, &errorMessage);
   if (!output)
     return failure();
 
   mlir::OwningModuleRef moduleRef;
   if (failed(loadModule(context, moduleRef, inputFilename)))

mlir/tools/mlir-reduce/CMakeLists.txt

-get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
 get_property(conversion_libs GLOBAL PROPERTY MLIR_CONVERSION_LIBS)
-
-if(MLIR_INCLUDE_TESTS)
-  set(test_libs
-    MLIRTestDialect
-    )
-endif()
+get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
 
 set(LIBS
-  ${dialect_libs}
   ${conversion_libs}
-  ${test_libs}
+  ${dialect_libs}
   MLIRDialect
   MLIRIR
   MLIRPass
   MLIRReduceLib
+  MLIRTestDialect
   )
 
 add_llvm_tool(mlir-reduce
   mlir-reduce.cpp
 
   DEPENDS
   ${LIBS}
   )
 
 target_link_libraries(mlir-reduce PRIVATE ${LIBS})
 llvm_update_compile_flags(mlir-reduce)
 
 mlir_check_all_link_libraries(mlir-reduce)