Page MenuHomePhabricator

Refactor the architecture of mlir-reduce
ClosedPublic

Authored by Chia-hungDuan on Apr 1 2021, 3:18 AM.

Details

Summary

Add iterator for ReductionNode traversal and use range to indicate the region we would like to keep. Refactor the interaction between Pass/Tester/ReductionNode.
Now it'll be easier to add new traversal type and OpReducer

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Chia-hungDuan requested review of this revision.Apr 1 2021, 3:18 AM
Chia-hungDuan edited the summary of this revision. (Show Details)Apr 1 2021, 3:19 AM
Chia-hungDuan added a reviewer: jpienaar.
Chia-hungDuan updated this revision to Diff 334641.EditedApr 1 2021, 3:50 AM

Add header for ReductionTreePass.cpp and fix some linting error

jpienaar edited the summary of this revision. (Show Details)Apr 1 2021, 7:05 AM
jpienaar added a reviewer: rriddle.
rriddle requested changes to this revision.Apr 1 2021, 4:29 PM

Thanks for starting to look at this! Please fix the clang-tidy issues.

Can you also re-upload with full context?

mlir/include/mlir/Reducer/Pass
35 ↗(On Diff #334641)

Why return vector here? Can you just return module.getOps<OpType>() directly?

48 ↗(On Diff #334641)

Can you use ArrayRef here instead?

55 ↗(On Diff #334641)

nit: Spell out auto here.

57 ↗(On Diff #334641)

nit: Please drop all trivial braces in this commit.

71 ↗(On Diff #334641)
mlir/include/mlir/Reducer/Passes/OpReducer.h
29

How is this different from the other OpReducer file?

mlir/include/mlir/Reducer/ReductionTreePass.h
48–49

Does this need to be on the pass instance? It looks like it gets reset every runOnOperation.

(Same thing with the allocator, why not have it local to runOnOperation?)

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

nit: Use the llvm:: variants of all_of/remove_if/etc.

125

Inner comments should use //.

mlir/tools/mlir-reduce/ReductionTreePass.cpp
24

Can we just make the implementation here not templated instead? Moving to a .cpp seems like it would limit the ability for out-of-tree users to utilize this.

45

Is something erasing these clones?

This revision now requires changes to proceed.Apr 1 2021, 4:29 PM
Chia-hungDuan marked 4 inline comments as done.

Fixed the review comments and removed weird file in the previous change

Chia-hungDuan marked 2 inline comments as done.Apr 6 2021, 12:27 AM
Chia-hungDuan added inline comments.
mlir/include/mlir/Reducer/Pass
35 ↗(On Diff #334641)

Just inline module.getOps<OpType>(), removed getSpecificOps

55 ↗(On Diff #334641)

Sorry, I think this nit was required when getSepcificOps() is returning vector. I've changed to use module.getOps<OpType>(), do you think we still need to unroll it? it will be (result_pair<llvm::iterator_range<mlir::detail::op_iterator<mlir::FuncOp, mlir::Region::OpIterator>>>).

71 ↗(On Diff #334641)

It seems that we don't need this, I removed this function. If we need this information, I think it should be involved in pass information

mlir/include/mlir/Reducer/Passes/OpReducer.h
29

Sorry, the above file("mlir/include/mlir/Reducer/Pass") was added by accident. The refactor of OpReducer was supposed to be in the follow up CL. I've moved the changes into this CL

mlir/include/mlir/Reducer/ReductionTreePass.h
48–49

Moved to local vars in runOnOperation.

Reduce the number of generating new variants and change to use llvm::ArrayRef while accessing variants vector.

jpienaar added inline comments.Apr 6 2021, 6:40 AM
mlir/include/mlir/Reducer/Passes/OpReducer.h
31

Could you add a comment describing the member function?

34

The previous approach here I see being generator like and towards having the same structure for all the reducers. So from the driver you'd have a common set of methods to call for each reducer. And so these now map to reduce and getNumTargetOps ?

39

It seems with this change we lose the ability to specify number of variants is that correct?

mlir/include/mlir/Reducer/ReductionNode.h
66

Nit: Already in private scope

76

I prefer public, protected and private ordering

mlir/include/mlir/Reducer/ReductionTreePass.h
52

Same re unneeded private

mlir/include/mlir/Reducer/Tester.h
38

I was thinking NotSure was like Maybe case, but it seems it is actually used to represent not tested yet, is that correct? Then perhaps Unexplored or Untested might be more intuitive

48

Check is overloaded for me (probably due to CHECK), prefer something like Return the whether file in the given path is interesting.

mlir/lib/Reducer/Tester.cpp
32

Why are you printing to temporary file here?

43

There is no bitcode here yet

mlir/tools/mlir-reduce/ReductionNode.cpp
51

maxElement?

56

Elide braces for trivial if bodies

106

Nit: remove this indentation, we don't have that style anywhere else here.

110

So here you are testing if any of the variants are either interesting or not? (just filtering out notsure).

mlir/tools/mlir-reduce/ReductionTreePass.cpp
26

Could TypeId be used here instead?

Chia-hungDuan marked 9 inline comments as done.

Add more algorithm comments and fix some lints/erros based on review comments.

mlir/lib/Reducer/Tester.cpp
32

This is used to let the test script exams the interestingness.

43

Changed to IR.

mlir/tools/mlir-reduce/ReductionNode.cpp
110

In SinglePath approach, we would like to select the most reduced variant and keep traversing from that node.
Yes, here's checking if all the variants generated from certain node are tested, if it's tested, then we can select the smallest variant, if not, we just need to wait for all the variants are tested.

I added more comments in explaining the approach.

mlir/tools/mlir-reduce/ReductionTreePass.cpp
26

We need a Op identifier but it seems that TypeID is bound to a specific value, right? If so, I think it may not be able to use in this case

jpienaar accepted this revision.Apr 8 2021, 8:46 AM

I think this looks good to go and we can always iterate

mlir/tools/mlir-reduce/ReductionNode.cpp
50

Left over from debugging?

108

Why +2?

mlir/tools/mlir-reduce/ReductionTreePass.cpp
26

I was thinking one could do something like

getOpReducer(TypeId opId) {

if (opId == TypeID::get<ModuleOp>())

Perhaps I'm not following, but it seems it would give us this same structure (as we only use opType to dispatch to a particular version). But then again string comparison here is probably not on the hot path

Remove debugging header

Chia-hungDuan added inline comments.Apr 8 2021, 10:59 PM
mlir/tools/mlir-reduce/ReductionNode.cpp
50

Removed. lol

108

vector::insert inserts an element before the given iterator and it returns the iterator points to the new element. Here we insert two split ranges so the one being split now is at it + 2

mlir/tools/mlir-reduce/ReductionTreePass.cpp
26

Yes, this one only be used once while pass initialization. OTOH, we will use command line to configure the reduction process(D100155), I will revisit this structure while we have more kinds of Reducer.

rriddle added inline comments.Apr 12 2021, 6:53 PM
mlir/include/mlir/Reducer/Passes/OpReducer.h
28

Can you add comments for this method? and what rangeToKeep contains?

29

nit: Drop the llvm:: here, ArrayRef is exported in the mlir namespace.

29

Should this std::pair be ReductionNode::Range?

42

Does this need to be templated? Can you pass in an OperationName to the constructor and use that for finding target ops instead?

48

Is this supposed to be in the entire module? getOps only operates on one level of the IR (i.e. in the most directly nested region).

mlir/include/mlir/Reducer/ReductionTreePass.h
52

nit: Drop the llvm:: here, StringRef is exported in the MLIR namespace.

mlir/tools/mlir-reduce/ReductionTreePass.cpp
22

LLVM/MLIR style prefers using namespace instead.

26

Inside of .cpp files, only classes should really be placed in namespaces. Functions should be marked as static and at the top level.

31

If Reducer isn't templated, we wouldn't need to abort here.

Chia-hungDuan marked 6 inline comments as done.

Fix linting erros.

@rriddle, about the OpReducer, I'm planing to have a registration structure for different type of Reducer in different projects. In this refactoring, I want to make the structure of graph traversal clear and extensible, I'll have another CL to address the Reducer soon. Do you think we can keep it for a short while?

Thanks

Chia-hung

rriddle accepted this revision.Apr 13 2021, 10:55 AM

Fix linting erros.

@rriddle, about the OpReducer, I'm planing to have a registration structure for different type of Reducer in different projects. In this refactoring, I want to make the structure of graph traversal clear and extensible, I'll have another CL to address the Reducer soon. Do you think we can keep it for a short while?

Thanks

Chia-hung

Yeah, if you are actively cleaning that up I'm okay with deferring.

LGTM

This revision is now accepted and ready to land.Apr 13 2021, 10:55 AM
This revision was automatically updated to reflect the committed changes.