Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[microNPU][2b] Create CascaderGraphs from TE graphs #9471

Merged
merged 7 commits into from
Jan 7, 2022

Conversation

mbaret
Copy link
Contributor

@mbaret mbaret commented Nov 8, 2021

RFC: apache/tvm-rfcs#37
Issue: #9429

The first step in the cascader is to create a CascaderGraph from a TE graph. To do this, every operator in the TE graph must get 'matched' by a Part matcher. This converts TE operations into cascader Parts by augmenting them with Propagators.

In this initial commit, we include basic Part matchers for ethosu_conv2d and some transform operators so that the graph creation can be tested.

@mbaret mbaret marked this pull request as draft November 8, 2021 17:42
@mbaret mbaret force-pushed the ethosu-cascader-3 branch 2 times, most recently from aba0a8e to 950f280 Compare December 22, 2021 10:54
@mbaret mbaret force-pushed the ethosu-cascader-3 branch 3 times, most recently from 867ef3a to 7a0cfb9 Compare January 5, 2022 16:00
@mbaret mbaret marked this pull request as ready for review January 5, 2022 16:00
The first step in the cascader is to create a
CascaderGraph from a TE graph. To do this, every
operator in the TE graph must get 'matched' by a
Part matcher. This converts TE operations into
cascader Parts by augmenting them with Propagators.

In this initial commit, we include basic Part
matchers for ethosu_conv2d and some transform
operators so that the graph creation can be tested.

Change-Id: I36bbbdbafd14f09e6d1a8abf32591d6755192915
Change-Id: I34e48e4ce4fd14e6c92601412f5029aa08efe8fc
Change-Id: Idcb92c26863837b06fa68f9122a1a79df79ee242
Change-Id: I0aad319b7722a56b5c7f116adcc791951223baa3
Change-Id: Ib1633341ed84e83c29c4220dd9bc4a7b6d17a5d0
Change-Id: I5223da462a8dadfda1798e1ea1c9b30b4e804217
Change-Id: Ib614c21e97723571aa002a2a4a3edda3afafb9d4
Copy link
Contributor

@jacobbohlin jacobbohlin left a comment

Choose a reason for hiding this comment

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

LGTM!

@mbaret
Copy link
Contributor Author

mbaret commented Jan 6, 2022

cc @Mousius could you take a look?

Copy link
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

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

Overall I think the code is non-controversial but test coverage needs a bit of work, could you look at adding the additional tests? It'd give confidence that as this evolves we don't lose some of the more specific clauses 😸

Comment on lines +256 to +273
write = output_tensor
if write.op.name != "ethosu_write":
return None
convert_to_nhcwb16 = write.op.input_tensors[0]
if convert_to_nhcwb16.op.name != "ethosu_convert_to_nhcwb16":
return None
conv2d = convert_to_nhcwb16.op.input_tensors[0]
if conv2d.op.name != "ethosu_conv2d":
return None
pad = conv2d.op.input_tensors[0]
if pad.op.name != "ethosu_pad":
return None
convert_to_nhwc = pad.op.input_tensors[0]
if convert_to_nhwc.op.name != "ethosu_convert_to_nhwc":
return None
read = convert_to_nhwc.op.input_tensors[0]
if read.op.name != "ethosu_read":
return None
Copy link
Member

Choose a reason for hiding this comment

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

I can't see test cases for all of these clauses, so we're missing most of the logic of this function? Can you add some?

part = match_ethosu_conv2d(out)

assert isinstance(part, cs.EthosuPart)
assert len(part.propagators) == 3
Copy link
Member

Choose a reason for hiding this comment

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

Can we test the other properties of the part returned?

part = match_ethosu_inline(out)

assert isinstance(part, cs.InlinePart)
assert len(part.propagators) == 1
Copy link
Member

Choose a reason for hiding this comment

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

Can we test the other properties of the part ?

)
pytest.importorskip("ethosu.vela")

import tvm.contrib.ethosu.cascader as cs
Copy link
Member

Choose a reason for hiding this comment

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

As much as I appreciate using my initials here, it'd be really great to use something I could immediately associate with the cascader; potentially cascader rather than cs ?

Comment on lines +44 to +45
assert part.propagators[0].transform == ifm_transform
assert part.propagators[0].offset == ifm_offset
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to test at least a few cases just to ensure the function doesn't always return the same thing 😸

Comment on lines +47 to +48
if output_tensor.op.name not in INLINE_OPS:
return None
Copy link
Member

Choose a reason for hiding this comment

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

Quick test for this please!

input_tensors = part.subgraph.input_tensors
break

assert part is not None, f"The tensor {tensor} doesn't match any part."
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to ensure this fires when we expect it to with pytest.raises.

Copy link
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

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

Had a chat with @mbaret, because this patch is blocking a bunch of other patches I'll commit this to enable forward progress and ensure that the test coverage is followed up on.

@Mousius Mousius merged commit afc29e6 into apache:main Jan 7, 2022
AndrewZhaoLuo pushed a commit to AndrewZhaoLuo/tvm that referenced this pull request Jan 7, 2022
The first step in the cascader is to create a
CascaderGraph from a TE graph. To do this, every
operator in the TE graph must get 'matched' by a
Part matcher. This converts TE operations into
cascader Parts by augmenting them with Propagators.

In this initial commit, we include basic Part
matchers for ethosu_conv2d and some transform
operators so that the graph creation can be tested.
ylc pushed a commit to ylc/tvm that referenced this pull request Feb 16, 2022
The first step in the cascader is to create a
CascaderGraph from a TE graph. To do this, every
operator in the TE graph must get 'matched' by a
Part matcher. This converts TE operations into
cascader Parts by augmenting them with Propagators.

In this initial commit, we include basic Part
matchers for ethosu_conv2d and some transform
operators so that the graph creation can be tested.
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