This is an archive of the discontinued LLVM Phabricator instance.

[mlir-reduce] Reducer refactor.
ClosedPublic

Authored by Chia-hungDuan on Apr 22 2021, 4:15 AM.

Details

Summary
  • A Reducer is a kind of RewritePattern, so it's just the same as

writing graph rewrite.

  • ReductionTreePass operates on Operation rather than ModuleOp, so that
  • we are able to reduce a nested structure(e.g., module in module) by
  • self-nesting.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Apr 22 2021, 4:15 AM
Chia-hungDuan requested review of this revision.Apr 22 2021, 4:15 AM

Handle empty region and adjusted some early return logics.

Fixed linting error and updated comment

Thanks! Took a first pass.

mlir/include/mlir/Reducer/ReducerRegistry.h
1 ↗(On Diff #339968)
18 ↗(On Diff #339968)

Can you document these?

mlir/include/mlir/Reducer/ReductionNode.h
65

Can you add documentation here?

71

Should you be using ArrayRef here instead?

94

Please document this.

mlir/lib/Reducer/ReductionNode.cpp
62–64
67–68

Given you are already constructing the vector, can you move it when creating the new node? To avoid the extra copy.

mlir/lib/Reducer/ReductionTreePass.cpp
31–32

We should generally try to avoid static variables whenever reasonably possible. Why does this need to be static?

39

Why can't you use the default constructor? Pass options are copied implicitly, so I wouldn't expect you to need this.

50

nit: Drop trivial braces here and below.

52–53

Can you move this (pattern construction) to the initialize method?

https://mlir.llvm.org/docs/PassManagement/#initialization

62–63

You would also need to ensure that the nested operation is isolated from above, otherwise it will assert.

64–66

Can you cache these? This could end up creating a lot of duplicate pass managers, whereas you should only need one of each op-type.

66

clone()?

85–86

Does this need to be on ReductionTreePass? Can you make this static to this file instead? Also, can you split up this function? It is quite large.

119

Drop the const here, ArrayRef has only const API.

183–187

Could this be pass failures instead of hard crashes?

mlir/test/lib/Reducer/MLIRTestReducer.cpp
41
Chia-hungDuan marked 11 inline comments as done.Apr 26 2021, 7:20 PM
Chia-hungDuan added inline comments.
mlir/include/mlir/Reducer/ReductionNode.h
65

Added the comment at the data member section.

mlir/lib/Reducer/ReductionTreePass.cpp
31–32

This is supposed to have the same function as passRegistry to let the users register various kinds of reducer patterns. The size will be far smaller than passRegistry in fact.

39

Done.
I thought copy constructor would do that, but found that Option are not copied. I forgot there's a clone() and it handles the Option properly.


std::unique_ptr<Pass> clone() const {
  auto newInst = clonePass();
  newInst->copyOptionValuesFrom(this);
  return newInst;
}

Do you think we need to add this in the copy ctor of Pass?

62–63

Thanks for reminder, I didn't notice that

64–66

Rewrite it without using OpPassManager, just do a operation walk recursively.

85–86

Split the lambda to a static function and clean the content in this function. Let's see if we still need to move it to static in next review.

183–187

These two are supposed to be always true. So if it's failed then means we have problems in the algorithm. Or do you think using an assertion will be better?

Add comments and did some structures clean up

rriddle added inline comments.Apr 27 2021, 6:40 PM
mlir/include/mlir/Reducer/ReductionNode.h
65

Can you add it here too? Data members aren't necessarily user facing API.

71

Drop the llvm::, ArrayRef is exported in the mlir namespace. Same applies throughout the revision,

mlir/lib/Reducer/ReductionNode.cpp
39–50

nit: You could just inline these into the header.

40
67–68

Using ArrayRef will still generate a copy?

71

Can you use getVariants().drop_begin?

mlir/lib/Reducer/ReductionTreePass.cpp
31–32

I don't understand your point, why can't the user just collect the registrations and pass them when constructing the pass? The pass registration mechanism is a bit of a special case (and could likely be restructured to not be static, albeit with a bit of work).

52

Drop the trivial braces here.

57

nit: Fix the naming style here.

93

nit: Fix the naming style here.

114

Can you make this a non-member of the class?

183–187

LLVM style is to assert liberally for invariants. If these things generally shouldn't happen, assert is fine.

mlir/tools/mlir-reduce/mlir-reduce.cpp
90

If we generate something invalid, couldn't we prevent that from escaping the pass boundary? Do you have an example use case that requires this?

Chia-hungDuan marked 9 inline comments as done.Apr 29 2021, 5:45 AM
Chia-hungDuan added inline comments.
mlir/lib/Reducer/ReductionNode.cpp
67–68

Ah, my bad. Did few revisions here and forgot the copy issue. Fixed

mlir/lib/Reducer/ReductionTreePass.cpp
31–32

I didn't find a better way to pass the patterns to construct a Pass.

Now I added a function ReductionTreePass::populateReducerPatterns to collect the patterns. It'll iterate over the pass manager and pass the patterns if it's a ReductionTreePass.

mlir/tools/mlir-reduce/mlir-reduce.cpp
90
  • The original ---

func @simple1(%arg0: i1, %arg1: memref<2xf32>, %arg2: memref<2xf32>) {

cond_br %arg0, ^bb1, ^bb2

^bb1:

br ^bb3(%arg1 : memref<2xf32>)

^bb2:

%0 = memref.alloc() : memref<2xf32>                                               
br ^bb3(%0 : memref<2xf32>)

^bb3(%1: memref<2xf32>):

return

}

  • reduced module ---

module {

func @simple1(%arg0: i1, %arg1: memref<2xf32>, %arg2: memref<2xf32>) {
  cond_br %arg0, ^bb1, ^bb2
^bb1:  // pred: ^bb0
^bb2:  // pred: ^bb0
^bb3(%0: memref<2xf32>):  // no predecessors
}

}

  • The error message ---

error: empty block: expect at least a terminator

This is the test case 'test/mlir-reduce/simple-test.mlir', the tester always return true so that we reduce it into this invalid form.

Yes, I'm thinking if we should always check the validity of the reduced form. Now I let the tester to decide if an invalid form is acceptable. What do you think?

Addressed review comments

Nice, first scan

mlir/include/mlir/Reducer/ReductionNode.h
38–42

ReductionTreePass

78

Lets be consistent here use "///" and comment all members.

161–168

represents the selected subset?

jpienaar added inline comments.Apr 29 2021, 8:50 AM
mlir/include/mlir/Reducer/ReductionNode.h
171

reduced

171

I think you have all the correct info here, perhaps we could make it shorter though. WDYT of

startRanges records the ranges of operations selected from the parent node to produce this ReductionNode. It can be used to construct the reduction path from the root.

(one could include the i.e., section still).

mlir/lib/Reducer/ReductionNode.cpp
64–66

Would getRange().size() == 0 be possible here? (it would probably not produce something post, I was just curios about != 1 vs > 1 here)

mlir/tools/mlir-reduce/mlir-reduce.cpp
90

I think that is a good idea: the input to test on should be valid, else we may be reducing a case where a pass produces an invalid IR, and we end up retaining the error message by creating a new invalid input with the same failure message instead.

102

You should be able to drop llvm:: here and drop the ReductionTreePass on lhs as we have it captured in the cast

Chia-hungDuan marked 6 inline comments as done.Apr 30 2021, 1:55 AM
Chia-hungDuan added inline comments.
mlir/include/mlir/Reducer/ReductionNode.h
171

It's more clear, thanks!

mlir/lib/Reducer/ReductionNode.cpp
64–66

getRange().size() is guaranteed to be >= 1. Yes, it won't cause any problem even it's empty and I agree > 1 will be better.

Addressed review comments

rriddle added inline comments.Apr 30 2021, 4:19 PM
mlir/include/mlir/Reducer/OptReductionPass.h
0

Do you actually need to declare this? I wouldn't expect it to be necessary.

mlir/lib/Reducer/ReductionTreePass.cpp
110

Why not just use SmallVector instead? It also has the nice pop_back_val API.

162

Do you need the double-ended nature of a queue? Or could you just use a SmallVector based stack?

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

What kind of patterns do we expect to be populated here? Would this be better served by a dialect interface of some kind? Where interested dialects could provide patterns for their (or related) operations?

Chia-hungDuan marked 3 inline comments as done.May 2 2021, 8:32 PM
Chia-hungDuan added inline comments.
mlir/include/mlir/Reducer/OptReductionPass.h
0

Removed

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

In the later CL(D101607) we change mlir-reduce to be a lib, for different project, they can provide their pattern s in the driver, for example, https://reviews.llvm.org/D101607#change-Zg1nOLxd11B7

For example, we may want to reduce a complex tensor into simpler form. I didn't have too many real cases(only got few samples from the codes which stays in the early stage of mlir-reduce), I'm planning to build tf-reduce in tensorflow and see what kinds of patterns may be useful.

Chia-hungDuan marked an inline comment as done.

Addressed review comment

rriddle added inline comments.May 4 2021, 3:30 PM
mlir/lib/Reducer/Tester.cpp
53

Can you add a comment here?

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

I think it would be nice to have a more principled way of providing patterns. The way this is structured right now feels a bit too adhoc. IMO the patterns should either be collected by a dialect interface, users defining their own reduction passes, or some other similar mechanism. Is their something in the structure of mlir-reduce that prevents those types of techniques or makes them harder to integrate with?

Chia-hungDuan marked an inline comment as done.May 7 2021, 3:40 AM

Added comments

LG, the dialect interface seems like a nice group mechanism

mlir/lib/Reducer/ReductionTreePass.cpp
139

Prefer: /*arg=*/ format to allow clang-tidy warnings if mismatched

162

perhaps marked done accidentally ?

Chia-hungDuan marked an inline comment as done.

Use Dialect Interface to collect reduce patterns

Verify the module before print it to the file because an invalid module may fail dump.

Looking good, thanks! May just need one more look over after this round, but should be really close to landing. Awesome work!

mlir/include/mlir/Reducer/OptReductionPass.h
0

Can we delete this file and just move the pass class into the .cpp file?

0

I don't think this include is necessary.

mlir/include/mlir/Reducer/ReducePatternInterface.h
18 ↗(On Diff #347301)

Can you beef up this documentation? It should be a bit more clear what the contract with the user is, i.e. if I was adding support for my own dialect, how am I supposed to use this interface?

Also, nit: Top-level docs should use ///.

22 ↗(On Diff #347301)

nit: Can you make this a protected member?

24 ↗(On Diff #347301)

Can you beef this description up? We should document what types of patterns should be provided here, and give an idea on how they are going to be used.

Also, nit: Top-level docs should use ///.

25 ↗(On Diff #347301)

nit on name (feel free to ignore): What about populateReductionPatterns?

mlir/include/mlir/Reducer/ReductionNode.h
149

Can you add a doc here?

mlir/include/mlir/Reducer/ReductionTreePass.h
0

Same comment as in OptReductionPass.h: Can we delete this file and move the pattern class into the .cpp file?

mlir/lib/Reducer/ReductionTreePass.cpp
148

Drop the extra //?

149

nit: Can you rename to ReducePatternInterfaceCollection? The current name leads me to believe this is an interface, as opposed to a collection of interfaces. Also, this class should be in an anonymous namespace.

mlir/lib/Reducer/Tester.cpp
31

Do we want to setup a dummy diagnostic handler here? I don't think we want the user to see errors generated here (at least not in the common case).

mlir/test/lib/Dialect/Test/TestInterfaces.h
37 ↗(On Diff #347301)

nit: Can you move this to TestDialect.cpp? The other test dialect interfaces are there as well: https://github.com/llvm/llvm-project/blob/7a211ed110a72ad453305aba250da50c965c2f8e/mlir/test/lib/Dialect/Test/TestDialect.cpp#L47

mlir/test/lib/Dialect/Test/TestPatterns.cpp
56

Can you just have this be a static method that the interface calls into? That would remove the need to define the interface in the header.

mlir/tools/mlir-reduce/mlir-reduce.cpp
16

Is this necessary? Can you trim some of these now?

32

Is this necessary now?

Chia-hungDuan marked 14 inline comments as done.May 26 2021, 4:01 AM
Chia-hungDuan added inline comments.
mlir/include/mlir/Reducer/OptReductionPass.h
0

File removed.

0

Done. In fact, I'm thinking to merge this into ReductionTreePass, maybe in the later CL

mlir/include/mlir/Reducer/ReducePatternInterface.h
25 ↗(On Diff #347301)

Done. In fact, this is an useful comment for me :)

mlir/lib/Reducer/Tester.cpp
31

Yes, I'm going to have another CL for improving the usage, doc also included. This will be covered in that CL

mlir/tools/mlir-reduce/mlir-reduce.cpp
32

Sorry, missed to clean this

Chia-hungDuan marked 4 inline comments as done.

Addressed review comments and fixed two bugs

  1. The reduction path construction should include the root node (Changes in ReductionTreePass.cpp:121)
  1. applyOpPatternsAndFold may erase operations, so it couldn't be in the enumerate loop (Changes in ReductionTreePass.cpp:61)
jpienaar accepted this revision.May 26 2021, 12:00 PM

Looks good, I'll let River take one more pass

mlir/include/mlir/Reducer/ReductionNode.h
41

s/.etc./etc./

55

Does this effectively mean it returns the most reduced Module that is interesting? E.g., if the pattern hasn't been applied then it is either due to the pattern not being applicable or result not being interesting? Or would it reduce even if the reduced one is no longer interesting? (I don't have much better suggestion, but getModule feels like a const accessor, it doesn't capture much)

78

variants (the child nodes)

91

For passes, we have initialize method, could we follow the same convention/spelling?

mlir/lib/Reducer/ReductionTreePass.cpp
62

Could you document why do we skip checking the result of folding?

72

ReductionNode

This revision is now accepted and ready to land.May 26 2021, 12:00 PM
Chia-hungDuan marked 5 inline comments as done.May 27 2021, 3:49 AM
Chia-hungDuan added inline comments.
mlir/include/mlir/Reducer/ReductionNode.h
55

A ReductionNode will be in either two states (Interestingness::Untested, Interestingness::True/Interestingness::False). If it's tested, the module will have been applied the patterns on the operations within the given ranges and that's the reduced module. The module may not be interesting or has smaller size than others like its parent. It just an exploration state. You're right, I shouldn't say it's reduced, I would say it's applied some reduction strategies.

According to our algorithm, we won't explore an uninteresting state, i.e., ReductionNode with Interestingness::False. If we have a ReductionNode with Interestingness::Untested, which means this node is waiting for testing interestingness.

Added const qualifier and fixed/beefed up the comment.

91

Done.
I used assert for ensuring the result should be success() for now, will update them to proper diagnostic in later CL

Chia-hungDuan marked an inline comment as done.

Addressed review comments and updated the commit message

rriddle accepted this revision.May 27 2021, 1:34 PM

Looks good, thanks!

mlir/include/mlir/Reducer/ReductionPatternInterface.h
44–46

?

47–49

?

mlir/lib/Reducer/OptReductionPass.cpp
80

Only classes should really go in anonymous namespaces, can you move this to the end of the class above?

mlir/lib/Reducer/ReductionTreePass.cpp
35

Only classes should really be in anonymous namespaces, the static functions should be top-scope. Can you restrict the anonymous namespace to just contain classes?

Chia-hungDuan marked 4 inline comments as done.May 28 2021, 3:02 AM
Chia-hungDuan added inline comments.
mlir/include/mlir/Reducer/ReductionPatternInterface.h
44–46

Very appreciated for revising the sentences!

Chia-hungDuan marked an inline comment as done.

Addressed review comments

Shouldn't put statement in assert or it may be removed in certain build.

This revision was automatically updated to reflect the committed changes.
mlir/tools/mlir-reduce/CMakeLists.txt