Page MenuHomePhabricator

Use PassPipelineCLParser in mlir-reduce
ClosedPublic

Authored by Chia-hungDuan on Apr 8 2021, 4:22 PM.

Details

Summary

We are able to config the reducer pass pipeline through command-line.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Apr 8 2021, 4:22 PM
Chia-hungDuan requested review of this revision.Apr 8 2021, 4:22 PM
Chia-hungDuan edited the summary of this revision. (Show Details)
Chia-hungDuan added reviewers: jpienaar, rriddle.
Chia-hungDuan added inline comments.Apr 8 2021, 4:29 PM
mlir/include/mlir/Reducer/Passes.td
28–31

I'm wondering if I could lift this to a ReductionBase, i.e.,

class ReductionBase : Pass<...> {

let options = [
   ...
]

}

ReductionTree : ReductionBase<...> {

...

}

Make ReductionTree inherits the options from base. It didn't work, I'm checking if I did anything wrong

This CL also erases the bug mentioned here, https://bugs.llvm.org/show_bug.cgi?id=48094

mlir/include/mlir/Reducer/Passes.td
28–31

Note: After checking the tblgen, it seems we don't support this feature.

Nice cleanup!

mlir/include/mlir/Reducer/Passes.td
28–31

I don't know if it's necessary to share here, but if you wanted to you could define a separate list and then concat them together:

// Op's regions that don't need a terminator: requires some other traits
// so it defines a list that must be concatenated.
def SharedReductionPassOptions {
  list<Option> options = [
    Option<"TesterName", "test", "std::string", /* default */"",
           "The filename of the tester">,
    ListOption<"TesterArgs", "test-arg", "std::string",
               "llvm::cl::ZeroOrMore, llvm::cl::MiscFlags::CommaSeparated">,
  ];
}

...

def OptReduction : Pass<"opt-reduction-pass", "ModuleOp"> {
  let summary = "A reduction pass wrapper for optimization passes";

  let constructor = "mlir::createOptReductionPass()";

  let options = [
    Option<"OptPass", "opt-pass", "std::string", /* default */"",
           "The optimization pass will be run dynamically in OptReductionPass">,
  ] # SharedReductionPassOptions.options;
}
mlir/tools/mlir-reduce/OptReductionPass.cpp
18

I think this header is already transitively included.

22

This change goes against the style, using namespace is preferred.

68
mlir/tools/mlir-reduce/mlir-reduce.cpp
93

What is the GG for here? (Good Game?)

93–94
Chia-hungDuan marked 4 inline comments as done.

Fixes from review comments

mlir/include/mlir/Reducer/Passes.td
28–31

Thanks! First time to know # can do this!

mlir/tools/mlir-reduce/mlir-reduce.cpp
93

lol, sorry, that's my bad. Left some verification codes...

rriddle accepted this revision.Apr 13 2021, 1:14 PM
rriddle added inline comments.
mlir/include/mlir/Reducer/Passes.td
20

The option names should be camelCase, as they correspond to C++ variable names.

mlir/tools/mlir-reduce/OptReductionPass.cpp
28

nit: Drop the const, we generally don't use it in situations like this.

This revision is now accepted and ready to land.Apr 13 2021, 1:14 PM
jpienaar accepted this revision.Apr 13 2021, 4:17 PM

Nice thanks, LG with River's comments ddressed

Chia-hungDuan marked 2 inline comments as done.

Fix naming

jpienaar edited the summary of this revision. (Show Details)Apr 14 2021, 2:33 PM
This revision was automatically updated to reflect the committed changes.