Skip to content

Conversation

@masahi
Copy link
Member

@masahi masahi commented Apr 19, 2019

See #3039 for the context. This is my take on fixing tuple fusion.

  • a tuple that is an intermediate value in its group can be fused. The existing test case passes unmodified.

  • a tuple that is the root in its group is lifted out of a subfunction. Before this PR, the tuple is the return value of the subfunction

Here is the approach:

  • First, tuples are marked as Injective so that they can be fused as much as possible.
  • After fusion step, chech if there are tuples that are roots in their groups
  • Update the data flow graph to detach root tuples from predecessors
  • Run DomTree construction and fusion step again. The tuples are no longer roots.

Please review @tqchen @zhiics @vinx13 @MarisaKirisame

assert relay.ir_pass.alpha_equal(zz, after)


def test_tuple_strided_slice():
Copy link
Member Author

@masahi masahi Apr 19, 2019

Choose a reason for hiding this comment

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

this test case is removed, because now this is basically the same test case as above test (test_tuple_root).

@tqchen
Copy link
Member

tqchen commented Apr 20, 2019

I hope we can use something that is more principled. Instead of the fuse then amend approach.

The general approach is that:

  • Mark TupleNode's pattern type as kTupleNode
  • Allow fuse kTupleNode into following injective nodes, in this case, the final group becomes kInjective
  • Do not allow fuse other nodes into kTupleNode

In this way, fusion will only occur if the tuple node gets fused into subsequent ones. We can do the traverse in backward order so that the effect of fused tuple node can propagate through backward.

@masahi
Copy link
Member Author

masahi commented Apr 20, 2019

ok, I'm not sure how "traverse and propagate backward" is supposed to work, but I'll give it another try.

I think the following approach should work:

In the first round, tuple can be fused into subsequent injective ops. So

one or more injective ops -> tuple -> one or more injective ops

becomes

[fused injective ops] -> [fused injective ops (+ tuple at the top)]

In the next round, we find that there are two consecutive injective ops that are not fused, so we fuse them. Thus, the intermediate tuple is fused with both preceding and subsequent ops.

If the tuple is a root in its group, it will remain unfused after the first round. The second round will also leave the tuple unfused, so we are good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants