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] Add transform matrices and part matcher to identity op #11453

Merged
merged 4 commits into from
Jun 1, 2022

Conversation

NicolaLancellotti
Copy link
Contributor

@NicolaLancellotti NicolaLancellotti commented May 25, 2022

This commit adds support for the identity operator in the cascader.

@manupa-arm @ekalda @lhutton1

cc @Mousius

@github-actions github-actions bot requested a review from Mousius May 25, 2022 14:00
Copy link
Contributor

@ekalda ekalda left a comment

Choose a reason for hiding this comment

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

Thanks @NicolaLancellotti, looks good in general! Just some comments about mapping 3D tensors to NHWC.

@ekalda
Copy link
Contributor

ekalda commented May 26, 2022

(I don't know why it displays each comment twice 😅 )

@NicolaLancellotti
Copy link
Contributor Author

(I don't know why it displays each comment twice )

No, problem. Thanks, @ekalda for the review.

@manupak
Copy link
Contributor

manupak commented May 30, 2022

Hi @NicolaLancellotti ,

It would be great to rebase this PR on top of https://github.com/apache/tvm/pull/11410/files and see if the unit tests that I have disabled in that PR works -- so we can have higher confidence about the changes in this PR.

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.

Looking forward to the rebase :) just a couple of small things that might be worth including?


subkernels = len(device_config.get_kernel_steps(identity.op.name, 1, 1, ifm_dtype))

input_layout = output_layout = "NHWC"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should assert that len(input_tensors_shape) <= 4 if we don't support brick layout for identity

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, apologies, I should have said that a message would have been helpful alongside the assert as well. Lets take it in a follow up :)

Copy link
Contributor

@ekalda ekalda left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

LGTM!


subkernels = len(device_config.get_kernel_steps(identity.op.name, 1, 1, ifm_dtype))

input_layout = output_layout = "NHWC"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, apologies, I should have said that a message would have been helpful alongside the assert as well. Lets take it in a follow up :)

@lhutton1 lhutton1 merged commit ee26ecf into apache:main Jun 1, 2022
@lhutton1
Copy link
Contributor

lhutton1 commented Jun 1, 2022

Thanks @NicolaLancellotti, @ekalda, @manupa-arm!

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