-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
b92a2a2
to
6e1b3d4
Compare
6e1b3d4
to
1279da0
Compare
aba0a8e
to
950f280
Compare
867ef3a
to
7a0cfb9
Compare
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
7a0cfb9
to
a6f2857
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
cc @Mousius could you take a look? |
There was a problem hiding this 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 😸
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
?
assert part.propagators[0].transform == ifm_transform | ||
assert part.propagators[0].offset == ifm_offset |
There was a problem hiding this comment.
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 😸
if output_tensor.op.name not in INLINE_OPS: | ||
return None |
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
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
.
There was a problem hiding this 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.
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.
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.
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.