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.

Sign up for GitHub

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

Jump to bottom

[microNPU] Add support for TFLite concatenate #9589

Merged
merged 5 commits into from
Dec 6, 2021
Merged

[microNPU] Add support for TFLite concatenate #9589

merged 5 commits into from
Dec 6, 2021

Conversation

ekalda
Copy link
Contributor

@ekalda ekalda commented Nov 25, 2021

  • Add legalization pass and is_valid checks for concatenate
  • Add TIR pass for removing concatenates and replacing them with direct
    writes to the final buffer
  • Add tests

Co-authored-by: Matthew Barrett [email protected]

@ekalda
Copy link
Contributor Author

ekalda commented Nov 25, 2021

@mbaret @manupa-arm @lhutton1 @dchauhan-arm @NicolaLancellotti

@ekalda ekalda changed the title [miroNPU] Add support for TFLite concatenate [microNPU] Add support for TFLite concatenate Nov 25, 2021
@ekalda ekalda mentioned this pull request Nov 25, 2021
Closed
15 tasks
NicolaLancellotti
NicolaLancellotti reviewed Nov 26, 2021
View reviewed changes
python/tvm/relay/backend/contrib/ethosu/tir/unary_elementwise.py
@@ -71,4 +71,5 @@ def get_unary_elementwise_params(stmt, producers, consumers):
),
output_pointer,
replace_pointer,
is_allocator,
Copy link
Contributor

Choose a reason for hiding this comment

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

is_allocator is missing in the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

python/tvm/relay/backend/contrib/ethosu/tir/pooling.py
@@ -88,4 +88,5 @@ def get_pooling_params(
),
output_pointer,
replace_pointer,
is_allocator,
Copy link
Contributor

Choose a reason for hiding this comment

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

is_allocator is missing in the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

python/tvm/relay/backend/contrib/ethosu/tir/transform.py
Comment on lines 60 to +65
None,
True,
Copy link
Contributor

Choose a reason for hiding this comment

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

The docstring for the last two return values is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ekalda
Copy link
Contributor Author

ekalda commented Nov 26, 2021

@NicolaLancellotti thanks for the review! :)

lhutton1
lhutton1 approved these changes Nov 28, 2021
View reviewed changes
Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

Thanks @ekalda, LGTM! Feel free to ignore or follow up the comments

python/tvm/relay/backend/contrib/ethosu/tir/passes.py Outdated
concat_buffer = replace_info.buffer
new_indices = list(stmt.indices)
new_indices[replace_info.axis] += replace_info.offset
# DODGY STORE NODE
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment would be good here to describe why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That comment is courtesy of @mbaret so I'm not exactly sure what makes this store node dodgy, but I updated that comment anyway :)

python/tvm/relay/op/contrib/ethosu.py

def is_valid(self):
"""Checks whether Concatenate has compatible attributes with the hardware"""
if not check_valid_dtypes(self.input_tensors, supported_dtypes=[np.int8]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we can't support any other data type for concatenate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uint8 should be supported as well, but I didn't enable it since we don't test it... It would be trivial to enable it in the future if we need to support TFLite 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense, thanks for clarifying :)

manupak
manupak approved these changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

LGTM!

Seems not a trivial implementation. :). Lets do a rebase -- so that we could get this in...

tests/python/contrib/test_ethosu/test_codegen.py Outdated
)

# Assumes only two runtime.Modules are created -- i.e. single offload module
imported_modules = compiled_models[0].executor_factory.lib.imported_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs changing after target hooks but you ll find it anyway after a rebase :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have resolved all the conflicts (for now) and all the tests pass (except the mean tests which we will fix)!

@ekalda ekalda force-pushed the concat_upstream branch 2 times, most recently from 539e21c to 1f779ce Compare December 2, 2021 11:14
@ekalda @mbaret
[miroNPU] Add support for TFLite concatenate
387fef8 
* Add legalization pass and is_valid checks for concatenate
* Add TIR pass for removing concatenates and replacing them with direct
  writes to the final buffer
* Add tests

Co-authored-by: Matthew Barrett <[email protected]>
@manupak manupak merged commit 260c95d into apache:main Dec 6, 2021
@manupak
Copy link
Contributor

manupak commented Dec 6, 2021

Thanks! @ekalda @lhutton1 @NicolaLancellotti @mbaret . This is merged now!

@ekalda ekalda deleted the concat_upstream branch December 7, 2021 09:15
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
@ekalda @mbaret @ylc
[microNPU] Add support for TFLite concatenate (apache#9589)
e1d9b49 
* Add legalization pass and is_valid checks for concatenate
* Add TIR pass for removing concatenates and replacing them with direct
  writes to the final buffer
* Add tests

Co-authored-by: Matthew Barrett <[email protected]>
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
@ekalda @mbaret @yangulei
[microNPU] Add support for TFLite concatenate (apache#9589)
209f4a5 
* Add legalization pass and is_valid checks for concatenate
* Add TIR pass for removing concatenates and replacing them with direct
  writes to the final buffer
* Add tests

Co-authored-by: Matthew Barrett <[email protected]>
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 12, 2022
@ekalda @mbaret @yangulei
[microNPU] Add support for TFLite concatenate (apache#9589)
0722f79 
* Add legalization pass and is_valid checks for concatenate
* Add TIR pass for removing concatenates and replacing them with direct
  writes to the final buffer
* Add tests

Co-authored-by: Matthew Barrett <[email protected]>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@ekalda @mbaret @ylc
[microNPU] Add support for TFLite concatenate (apache#9589)
6c49a1b 
* Add legalization pass and is_valid checks for concatenate
* Add TIR pass for removing concatenates and replacing them with direct
  writes to the final buffer
* Add tests

Co-authored-by: Matthew Barrett <[email protected]>
qsqqsqqsq-intellif pushed a commit to qsqqsqqsq-intellif/tvm that referenced this pull request Apr 29, 2022
@ekalda @mbaret
[microNPU] Add support for TFLite concatenate (apache#9589)
aaeceee 
* Add legalization pass and is_valid checks for concatenate
* Add TIR pass for removing concatenates and replacing them with direct
  writes to the final buffer
* Add tests

Co-authored-by: Matthew Barrett <[email protected]>
@driazati driazati mentioned this pull request Jul 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhutton1 lhutton1 lhutton1 approved these changes

@manupak manupak manupak approved these changes

@NicolaLancellotti NicolaLancellotti NicolaLancellotti approved these changes

@anijain2305 anijain2305 Awaiting requested review from anijain2305

@areusch areusch Awaiting requested review from areusch

@comaniac comaniac Awaiting requested review from comaniac

@icemelon icemelon Awaiting requested review from icemelon

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@jwfromm jwfromm Awaiting requested review from jwfromm

@MarisaKirisame MarisaKirisame Awaiting requested review from MarisaKirisame

@mbrookhart mbrookhart Awaiting requested review from mbrookhart

@merrymercy merrymercy Awaiting requested review from merrymercy

@slyubomirsky slyubomirsky Awaiting requested review from slyubomirsky

@tqchen tqchen Awaiting requested review from tqchen

@vinx13 vinx13 Awaiting requested review from vinx13

@wweic wweic Awaiting requested review from wweic

@yzhliu yzhliu Awaiting requested review from yzhliu

@zhiics zhiics Awaiting requested review from zhiics

@ZihengJiang ZihengJiang Awaiting requested review from ZihengJiang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ekalda @manupak @NicolaLancellotti @lhutton1