[NNVM] Enhance operator fusion for more element wise patterns #1548

masahi · 2018-08-03T12:45:59Z

Relevant discussion is here.

@tqchen @srkreddy1238 @PariksheetPinjari909 @merrymercy please review.

This PR enhances NNVM operator fusion to support certain element wise patterns, like the one below. This pattern occurs when exponential linear unit is used, for example (see the link above for more info).

This PR enables to fuse all operators above into a single operator. The followings patterns are supported.

Any number of child nodes from the top node
The path from the top node to bottom node can contain any number of element wise ops.

The restrictions:

All branches must meet at the bottom node.
'In-between' nodes cannot have more than one child.

NNVM can already fuse operators in red below. I added a simple post processing that

Check if all children nodes are fused into a single op by the existing fusion algorithm
Fuse the parent node to children nodes. and update its group id to be the children's group id
If the parent node originally belongs to another group (for example, conv + batch norm), propagate the new group id to grand parent and upward

TOPI schedules for all backend are updated to prevent scheduling the same op more than once. This is required because the same op can be reached from more than one route during output-to-input traversal.

A test cast added at nnvm/tests/python/compiler/test_op_fusion.py.

tqchen · 2018-08-04T17:38:46Z

please resync the changes against the current master as in #1394.

masahi · 2018-08-04T20:26:41Z

@tqchen done

tqchen

some comments. The topi changes looks fine to me, maybe we can bring the topi changes in first

tqchen · 2018-08-04T20:56:41Z

nnvm/src/compiler/graph_fuse.cc

+      const auto& group_ids = children_group_ids[nid];
+      if (group_ids.size() <= 1) continue;
+      const auto child_group_id = group_ids[0];
+      // fuse this node with children if all children belong to the same group


We need a stronger rule, currently, simply merges all of the children may not be good for future operations

I can add a check to see if all children are elem wise ops.

But the only case I fuse a parent node with children is when the parent has multiple children, and all children nodes are already fused by the existing algorithm. I think this only occurs when all children are elem wise ops.

Do you have a counter example that fails my logic?

I think it is fine for now, but we still want to be careful and explicit here, in case there are future updates to the code that brings the rule

tqchen · 2018-08-04T20:57:12Z

nnvm/src/compiler/graph_fuse.cc

@@ -161,6 +161,44 @@ nnvm::Graph GraphFusePartition(nnvm::Graph g) {
      }
    }
  }
+
+  if (opt_level >= 1) {
+    std::vector<std::vector<uint32_t>> children_group_ids(idx.num_nodes());


we need to document more on the algorithm here. So far I am not certain about the entire logic

I explained my logic in the description of this PR, is there something not clear?
But I agree that code alone is not enough to understand what I am trying to do here.

OK, can you also add comment block about the logic being used in here?

sure, will do.

masahi · 2018-08-04T22:54:58Z

@tqchen TOPI update is split to #1556. After that PR is merged, I'll rebase this PR.

masahi · 2018-08-05T00:38:38Z

@tqchen Check for op pattern and doc added.

tqchen · 2018-08-05T02:52:04Z

nnvm/src/compiler/graph_fuse.cc

+        propagate the new group id to a grand parent and upward
+  */
+  if (opt_level >= 1) {
+    std::vector<std::vector<uint32_t>> children_group_ids(idx.num_nodes());


-> > > to avoid problem in old compilers

tqchen · 2018-08-05T02:53:03Z

@srkreddy1238 @kevinthesun @zhiics can you also take a look at this PR?

masahi · 2018-08-05T03:01:02Z

Please let me know if there is a counter example that refutes this PR !!

tqchen · 2018-08-05T03:12:58Z

To be clear, I think it is a great PR to introduce the logic, since this logic is important, we want to make sure that there is more than one reviewer in the community who can read it and appreciate what is going on:) Thanks for adding the detailed comments

merrymercy · 2018-08-05T04:56:15Z

nnvm/tests/python/compiler/test_op_fusion.py

+        _, params2 = utils.create_workload(sym2, 1, dshape[1:], seed=0)
+        output1 = build_and_run(sym1, params1, data, oshape, target, ctx, opt_level=2)
+        output2 = build_and_run(sym2, params2, data, oshape, target, ctx, opt_level=0)
+        np.testing.assert_allclose(output1, output2, rtol=1e-5, atol=1e-5)


We should check graph.index.num_nodes here to make sure correct fusion happens when opt_level=2, like the above test cases.

Nice! I couldn't find a way to check the number of nodes other than eyeballing.

kevinthesun · 2018-08-06T05:59:39Z

nnvm/src/compiler/graph_fuse.cc

+     * Any number of child nodes from the top node
+     * The path from the top node to bottom node can contain any number of element wise ops.
+
+     The only restriction is that in-between nodes cannot have more than one child.


What will happen if in-between nodes have parents not belonging to this block, such as skip-connection block?

My rule only applies when all in-between children nodes and the bottom node are fused by existing fusion algorithm (see my figure above). If some in-between node has more than one parent, existing fusion algorithm cannot fuse this node with other in-between nodes.

For example, if you add another parent node to the right in-between node 'ReLU' in my figure, nothing can be fused. Hope this makes sense.

Just to be sure, I check that in-between node has only one parent, here.

So for example, my rule does nothing to Resnet.

merrymercy · 2018-08-06T21:42:29Z

I have a comment in your topi PR #1556

tqchen · 2018-08-08T17:07:56Z

Thanks @masahi @merrymercy @kevinthesun This is merged!

masahi force-pushed the improve-op-fusion branch 3 times, most recently from 3020877 to 494b3b9 Compare August 4, 2018 00:30

tqchen added the status: need review label Aug 4, 2018

tqchen force-pushed the master branch from e316f03 to 7b59b8e Compare August 4, 2018 17:14

masahi force-pushed the improve-op-fusion branch 2 times, most recently from ee45b8f to 371d2eb Compare August 4, 2018 17:56

tqchen requested changes Aug 4, 2018

View reviewed changes

tqchen self-assigned this Aug 4, 2018

tqchen added status: review in progress status: need update need update based on feedbacks status: need review and removed status: need review labels Aug 4, 2018

masahi mentioned this pull request Aug 4, 2018

[TOPI] Prevent scheduling the same operation more than once #1556

Merged

masahi force-pushed the improve-op-fusion branch from 371d2eb to 7b12ec1 Compare August 4, 2018 23:19

tqchen requested changes Aug 5, 2018

View reviewed changes

merrymercy requested changes Aug 5, 2018

View reviewed changes

merrymercy approved these changes Aug 5, 2018

View reviewed changes

masahi mentioned this pull request Aug 5, 2018

Separate fusion from compilation/lowering as a standalone pass #1540

Closed

kevinthesun reviewed Aug 6, 2018

View reviewed changes

masahi added 2 commits August 8, 2018 10:57

enhanced op fusion for elu

296a050

don't fuse when op is opaque type

dfbfd46

masahi added 9 commits August 8, 2018 10:57

add elu test cast

b8662d9

update test

c658bcc

update test

4fe634f

update test

dd31822

add explicit check for elemwise ops

5763ccf

add doc for algorithm

874feda

fix for old compilers

9af8646

check number of nodes after fusion

3a3da5e

update traverse inline logic for arm cpu

e0dff61

masahi force-pushed the improve-op-fusion branch from b6b35f7 to e0dff61 Compare August 8, 2018 01:57

tqchen approved these changes Aug 8, 2018

View reviewed changes

tqchen merged commit 1ed28ae into apache:master Aug 8, 2018

tqchen added status: accepted and removed status: need review status: need update need update based on feedbacks status: review in progress labels Aug 8, 2018

masahi mentioned this pull request Aug 15, 2018

[TOPI, CUDA] Bug fix properly bind gpu threads to injective op #1603

Closed

masahi mentioned this pull request Feb 5, 2020

Fast exponent #4790

Merged

masahi mentioned this pull request Apr 15, 2020

Pattern Language, Matcher, Rewriter, and Function Paritioner #5231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NNVM] Enhance operator fusion for more element wise patterns #1548

[NNVM] Enhance operator fusion for more element wise patterns #1548

masahi commented Aug 3, 2018 •

edited

Loading

tqchen commented Aug 4, 2018

masahi commented Aug 4, 2018

tqchen left a comment

tqchen Aug 4, 2018

masahi Aug 4, 2018

tqchen Aug 4, 2018

tqchen Aug 4, 2018

masahi Aug 4, 2018

tqchen Aug 4, 2018

masahi Aug 4, 2018

masahi commented Aug 4, 2018

masahi commented Aug 5, 2018

tqchen Aug 5, 2018

tqchen commented Aug 5, 2018

masahi commented Aug 5, 2018 •

edited

Loading

tqchen commented Aug 5, 2018

merrymercy Aug 5, 2018

masahi Aug 5, 2018

masahi Aug 5, 2018

kevinthesun Aug 6, 2018

masahi Aug 6, 2018 •

edited

Loading

merrymercy commented Aug 6, 2018

tqchen commented Aug 8, 2018

[NNVM] Enhance operator fusion for more element wise patterns #1548

[NNVM] Enhance operator fusion for more element wise patterns #1548

Conversation

masahi commented Aug 3, 2018 • edited Loading

tqchen commented Aug 4, 2018

masahi commented Aug 4, 2018

tqchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masahi commented Aug 4, 2018

masahi commented Aug 5, 2018

Choose a reason for hiding this comment

tqchen commented Aug 5, 2018

masahi commented Aug 5, 2018 • edited Loading

tqchen commented Aug 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masahi Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

merrymercy commented Aug 6, 2018

tqchen commented Aug 8, 2018

masahi commented Aug 3, 2018 •

edited

Loading

masahi commented Aug 5, 2018 •

edited

Loading

masahi Aug 6, 2018 •

edited

Loading