Skip to content

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 Matthew.Barrett@arm.com

@ekalda
Copy link
Contributor Author

ekalda commented Nov 25, 2021

@ekalda ekalda changed the title [miroNPU] Add support for TFLite concatenate [microNPU] Add support for TFLite concatenate Nov 25, 2021
),
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

),
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

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! :)

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

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 :)


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 :)

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...

)

# 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
* 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 <Matthew.Barrett@arm.com>
@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
* 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 <Matthew.Barrett@arm.com>
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
* 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 <Matthew.Barrett@arm.com>
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 12, 2022
* 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 <Matthew.Barrett@arm.com>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* 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 <Matthew.Barrett@arm.com>
qsqqsqqsq-intellif pushed a commit to qsqqsqqsq-intellif/tvm that referenced this pull request Apr 29, 2022
* 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 <Matthew.Barrett@arm.com>
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.

4 participants