[Relay, OpFusion] Better tuple fusion implementation

[masahi]

See #3039 for the context and discussion.

This is my second cut at fixing tuple fusion, which I hope is a better approach than the rather ad hoc one in #3049 .

• Added two new op patterns, for marking tuple and tuple field nodes
• Added a new attribute to the fusion group structure, to keep track of data flow between groups (think hypergraph where nodes correspond to fusion groups)
• After the existing fusion step, check each fusion group to see if we can further fuse the group and its input groups. By now, we can determine If the tuple has been fused to subsequent ops or not. We can fuse tuple fields into the tuple here.

[masahi]

@tqchen I need to fix the tensorflow test, but can you have a look at the patch and let me know if I am on the right track?

[tqchen]

I think we can still simplify it. We do not need to record the inputs of the group. We can still just do the traversal in the node relation group.

If there is a relation x -> tuple, and we can directly check if the group of the tuple is already injective, which means it is already fused to another op. Then we can fuse the group of x and group of tuple, if the group of the x is injective, and everything in between x and tuple follows the same pattern

[masahi]

thanks, I was also able to remove kTupleFields, one of two new op patterns I added. I think it is much simpler now.

[tqchen]

@MarisaKirisame @vinx13 please help review this PR

[masahi]

Ready for review @tqchen @jroesch @MarisaKirisame @zhiics @vinx13

[tqchen]

Given that there is a lot of recent interest in the fusor, I opened a new issue for better docs #3109

[tqchen]

I think we need to justify a bit why put kTuple as 2. Because tuple is special, I would rather put it say like 7, and use

pattern <= kInjective || pattern == kTuple

to indicate the pattern is tuple aware

[masahi]

I thought the pattern of tuple needs to be smaller than kInjective, because when we fuse the tuple into subsequent injective ops, we want the pattern of fused group to be kInjective (CombinePattern returns the larger of the two patterns begin combined).

But I realized that CombinePattern is only called when child group's master ref is non null, so it doesn't work the way I expected. kTuple doesn't have to be smaller than kInjective.

[masahi]

It also means even if injective ops are fused into a broadcast op, the combined pattern is still kBroadcast. Is this intended?

[tqchen]

I see, never mind, we can keep this as it is then. Please add a comment on why we choose such order

[masahi]

I've just changed the kTuple to 7, no change to fuse_ops.cc was needed. I also prefer this since it makes the diff smaller

[tqchen]

Let us update CombinePattern to specially handle tuple + injective and tuple + broadcast

[masahi]

do you mean, both injective and broadcast win against tuple, even though their op pattern is smaller? (now kTuple is 7)

[masahi]

@tqchen do you want to update both CombinePattern and its call site? (it seems CombinePattern is only called when one of its arg is kOutEWiseFusable.)

[zhiics]

LGTM

[zhiics]

LGTM

[masahi]

@vinx13 @tqchen looks like CI is broken, I'm getting error from topi group conv2d test (verify_group_conv2d_NCHWc_int8). Maybe #3070 is related?

http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/PR-3092/5/pipeline/235

[tqchen]

Thanks @masahi @vinx13 @zhiics , this is merged