Separate fusion and Compilation #1564

zhiics · 2018-08-07T18:12:24Z

This PR decouples fusion and compilation which has been discussed in the issue #1540.

Currently, lowering and fusion are implemented in the same pass. It would be more appropriate to make fusion and compilation as separate passes. This has the following benefits 1) fusion could be applied individually (even multiple times) in the future when necessary, 2) fusion could be target-independent while compilation (or lowering) is target-dependent.

In the future, we might need to make fusion more extendable so that different fusing rules could be applied in a more convenient manner.

This PR added graph_fuse.h to contain the fusion related structures and moved compilation related code to graph_compile.cc

yzhliu · 2018-08-07T18:44:39Z

@masahi Could you also help to do a review?

masahi · 2018-08-07T20:12:04Z

nnvm/python/nnvm/compiler/build_module.py

@@ -299,7 +299,8 @@ def build(graph, target=None, shape=None, dtype="float32",
            graph._set_json_attr("opt_level", 0, "int")
        graph = graph.apply("InferShape").apply("InferType")
        with target:
-            graph = graph.apply("GraphFusePartition").apply("GraphFuseCompile")
+            graph = graph.apply("GraphFusePartition").apply(
+                "GraphFuse").apply("GraphCompile")


I'd suggest move

graph = graph.apply("GraphFusePartition").apply( "GraphFuse")

to outside of with target above, as fusion should be target independent.

And separate it into two lines,

graph = graph.apply("GraphFusePartition") graph = graph.apply("GraphFuse")

because in the future we want to add other fusion rules between "GraphFusePartition" and "GraphFuse".

I think "GraphFusePartition" should be renamed to something like "GraphFindFusibleGroups". It makes the separation of work between "Partition step" and "Fusion step" clearer. What do you think?

Good point to move fusion out of target:)

I was also thinking a good name for GraphFusePartition as well. I was thinking of "GraphFusionSetup", because it basically sets up the vectors... Anyway, I think "GraphFindFusibleGroups" also works.

masahi · 2018-08-07T20:15:47Z

nnvm/src/compiler/graph_fuse.cc

-  // The corresponding function.
-  GraphFunc compiled_func;
-};
-
 // Fuse the partitioned graph into segments.
 // Create a new graph with fused noded.


noded -> nodes

masahi · 2018-08-07T20:16:39Z

nnvm/src/compiler/graph_fuse.cc

-  // The corresponding function.
-  GraphFunc compiled_func;
-};
-
 // Fuse the partitioned graph into segments.
 // Create a new graph with fused noded.
 // Also inheritate attribute shape, dltype from previous graph.


inheritate -> inherit

masahi · 2018-08-07T20:22:42Z

nnvm/src/compiler/graph_fuse.cc

  // specially handle assign
  const nnvm::Op* assign_op = nnvm::Op::Get("_assign");

-  std::vector<FuseEntry> fuse_vec(idx.num_nodes());
+  FuseVec fuse_vec(idx.num_nodes());


To avoid confusion, rename fuse_vec to fuse_entries or something similar. The name 'fuse_vec' is also used in GraphFusePartition with a completely different type and sementics.

masahi

I know some of the issues are left-over from the original code, but let's fix them this time.

masahi · 2018-08-07T20:31:18Z

nnvm/src/compiler/graph_compile.cc

+  const DTypeVector& dtype_vec = g.GetAttr<DTypeVector>("dtype");
+  const GroupVec& group_vec = g.GetAttr<GroupVec>("group_root");
+  const MasterVec &master_vec = g.GetAttr<MasterVec>("group_master");
+  const PatternVec &pattern_vec = g.GetAttr<PatternVec>("pattern");


Put & after type name.

My clang format does this automatically. I will change the format-style...

masahi · 2018-08-07T20:33:03Z

nnvm/src/compiler/graph_compile.cc

+  // Start lowering
+  Array<tvm::LoweredFunc> func_list;
+  std::unordered_set<const tvm::Node*> func_set;
+  const IndexedGraph &idx = g.indexed_graph();


masahi · 2018-08-07T20:34:56Z

nnvm/src/compiler/graph_compile.cc

+    const auto& inode = idx[nid];
+    uint32_t new_nid = new_idx.node_id(kv.second.get());
+    if (inode.source->op() == assign_op) {
+      // Check if rhs of assign can be comute inplace


comute -> computed

masahi · 2018-08-07T20:36:04Z

nnvm/src/compiler/graph_compile.cc

+      } else {
+        assign_flag[new_nid] = 1;
+      }
+          }


fix this weird } position.

masahi · 2018-08-07T20:37:39Z

nnvm/src/compiler/graph_fuse.cc

-  // setup ref counter
+nnvm::Graph GraphFuse(nnvm::Graph&& g) {
+  CHECK(g.HasAttr("group_root") && g.HasAttr("pattern"))
+      << "GraphFusePartition pass hasn't been applied yet.";


If you rename "GraphFuseParition", make sure reflect that change here.

masahi · 2018-08-07T20:38:05Z

nnvm/src/compiler/graph_fuse.cc

-  std::string target = g.GetAttr<std::string>("target");
-  std::string target_host;
+  const GroupVec& group_vec = g.GetAttr<GroupVec>("group_root");
+  const PatternVec &pattern_vec = g.GetAttr<PatternVec>("pattern");


masahi · 2018-08-07T21:03:28Z

nnvm/src/compiler/graph_compile.cc

+  const PatternVec &pattern_vec = g.GetAttr<PatternVec>("pattern");
+
+  CHECK(g.HasAttr("fused_entries")) << "Fusion hasn't been applied yet.";
+  FuseVec fuse_vec = g.MoveCopyAttr<FuseVec>("fused_entries");


Rename fuse_vec. See my comment at graph_fuse.cc

masahi

Looks good. Thanks for refactoring.

yzhliu · 2018-08-08T00:52:27Z

nnvm/src/compiler/graph_compile.cc

+  return g;
+}
+
+nnvm::Graph GraphCompile(nnvm::Graph g) {


looks like can pass by const nnvm::Graph& g

Thanks. The reason why I didn't pass const ref was because I was using MoveCopyAttr in the body. But anyway, passing by reference for a graph is better. I will change the body accordingly.

yzhliu · 2018-08-08T01:07:00Z

nnvm/src/compiler/graph_compile.cc

+    .set_body(GraphCompile)
+    .depend_graph_attr("shape")
+    .depend_graph_attr("dtype")
+    .depend_graph_attr("fused_entries")


I suggest to avoid plural form, use "fused_entry" instead

yzhliu · 2018-08-08T20:53:16Z

Thanks @zhiics and @masahi , this is now merged.
@tqchen Looks like the CI is encountering some unstable issue, sometimes openblas complains about too many thread created.

zhiics added 3 commits August 7, 2018 10:47

Separate fusion and compilation

6745929

fix description of graph_fuse.h

cbe2e8b

fix lint

55c29b3

yzhliu self-assigned this Aug 7, 2018

yzhliu added the status: need review label Aug 7, 2018

masahi reviewed Aug 7, 2018

View reviewed changes

masahi requested changes Aug 7, 2018

View reviewed changes

masahi reviewed Aug 7, 2018

View reviewed changes

nnvm/src/compiler/graph_compile.cc Outdated

} else {

assign_flag[new_nid] = 1;

}

}

Copy link

Member

masahi Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix this weird } position.

masahi reviewed Aug 7, 2018

View reviewed changes

fix @masahi 's comments, move fusion out of target

f0da460

masahi approved these changes Aug 7, 2018

View reviewed changes

yzhliu added status: review in progress and removed status: need review labels Aug 8, 2018

yzhliu reviewed Aug 8, 2018

View reviewed changes

zhiics and others added 7 commits August 7, 2018 20:14

fix graph passing and make fused_entries singula in graph attr

544768e

fix typo

49e941a

fix some comments

6f4afb4

run test again

8679fbe

Merge branch 'master' into master

aea114d

remove rvalue for graphfuse and graphfindfusiablegroups

e792d31

Merge branch 'master' into master

165b02c

yzhliu approved these changes Aug 8, 2018

View reviewed changes

yzhliu merged commit 53d2431 into apache:master Aug 8, 2018

yzhliu added status: accepted and removed status: review in progress labels Aug 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate fusion and Compilation #1564

Separate fusion and Compilation #1564

zhiics commented Aug 7, 2018

yzhliu commented Aug 7, 2018

masahi Aug 7, 2018

zhiics Aug 7, 2018

masahi Aug 7, 2018

masahi Aug 7, 2018

masahi Aug 7, 2018 •

edited

masahi left a comment

masahi Aug 7, 2018

zhiics Aug 7, 2018 •

edited

masahi Aug 7, 2018

masahi Aug 7, 2018

masahi Aug 7, 2018

masahi Aug 7, 2018

masahi Aug 7, 2018

masahi Aug 7, 2018

masahi left a comment

yzhliu Aug 8, 2018

zhiics Aug 8, 2018

yzhliu Aug 8, 2018

yzhliu commented Aug 8, 2018

Separate fusion and Compilation #1564

Separate fusion and Compilation #1564

Conversation

zhiics commented Aug 7, 2018

yzhliu commented Aug 7, 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 7, 2018 • edited

Choose a reason for hiding this comment

masahi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiics Aug 7, 2018 • edited

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 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

yzhliu commented Aug 8, 2018

masahi Aug 7, 2018 •

edited

zhiics Aug 7, 2018 •

edited