Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
jpienaar added inline comments.Apr 29 2021, 8:50 AM
mlir/include/mlir/Reducer/ReductionNode.h
165

reduced

165

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
61–63

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
97

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.

109

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
165

It's more clear, thanks!

mlir/lib/Reducer/ReductionNode.cpp
61–63

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
25–26

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
100

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
25–26

Removed

mlir/tools/mlir-reduce/mlir-reduce.cpp
100

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
100

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
20

I don't think this include is necessary.

24

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

mlir/include/mlir/Reducer/ReducePatternInterface.h
18

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

nit: Can you make this a protected member?

24

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

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

mlir/include/mlir/Reducer/ReductionNode.h
145

Can you add a doc here?

mlir/include/mlir/Reducer/ReductionTreePass.h
33

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
147

Drop the extra //?

148

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

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
57

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
18

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

38

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
20

File removed.

24

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

mlir/include/mlir/Reducer/ReducePatternInterface.h
25

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
38

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
40

s/.etc./etc./

54

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)

74

variants (the child nodes)

87

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
54

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.

87

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
43–45 ↗(On Diff #348211)

?

46–48 ↗(On Diff #348211)

?

mlir/lib/Reducer/OptReductionPass.cpp
70

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
43–45 ↗(On Diff #348211)

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.