Skip to content

[TOSA] Replace Linear lowering of using Matmul with Conv2d #1336

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
Closed
wants to merge 1 commit into from
Closed

[TOSA] Replace Linear lowering of using Matmul with Conv2d #1336

wants to merge 1 commit into from

Conversation

@Jerry-Ge
Copy link
Contributor

@Jerry-Ge Jerry-Ge commented Dec 1, 2023

  • The existing Vela compiler doesn't support Matmul and TOSA.Fully_Connected will be deprecated
  • Also add support for linear layers with rank>2 + tests

@pytorch-bot
Copy link

pytorch-bot bot commented Dec 1, 2023 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/1336

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 78d0594 with merge base d96c49e (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 1, 2023
@Jerry-Ge Jerry-Ge mentioned this pull request Dec 2, 2023
Closed
digantdesai
digantdesai reviewed Dec 4, 2023
View reviewed changes
Copy link
Contributor

@digantdesai digantdesai left a comment

Choose a reason for hiding this comment

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

LGTM at a high level, let's close the comments?

backends/arm/operators/op_addmm.py
[bias_reshape_res.name, mm_res.name],
[add_res.name],
None,
TosaOp.Op().CONV2D,
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the rationale to use conv2d instead? Is it to support U55 better through older TOSA?
Any perf impact?

Also document that the input is in NHWC and weights are in OHWI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

U55 doesn't support matmul from two activation channels (it has operations which are activation, weights as input). This should also be faster generally as weight inputs are typically optimised.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. IIUC, should we lower something like torch.[b]mm to this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. IIUC, should we lower something like torch.[b]mm to this as well?

i think so.

@digantdesai digantdesai added partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Dec 4, 2023
robell
robell reviewed Dec 5, 2023
View reviewed changes
@Jerry-Ge
[TOSA] Replace Linear lowering of using Matmul with Conv2d
78d0594 
- Rationale: Vela compiler doesn't support Matmul and TOSA.Fully_Connected will be deprecated
- Also added support for linear layers with rank>2 and tests
- Temporarily removed the DuplicateDequantNodePass since the transform
  API now forces verifier but haven't supported quantized ops yet

Signed-off-by: Jerry Ge <jerry.ge@arm.com>
digantdesai
digantdesai approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@digantdesai digantdesai left a comment

Choose a reason for hiding this comment

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

LGTM, let me know if you are ready I can merge it. Thanks @Jerry-Ge

@facebook-github-bot
Copy link
Contributor

@digantdesai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Jerry-Ge
Copy link
Contributor Author

Jerry-Ge commented Dec 7, 2023

LGTM, let me know if you are ready I can merge it. Thanks @Jerry-Ge

thanks Digant! it's ready and let's merge it.

@Jerry-Ge Jerry-Ge mentioned this pull request Dec 7, 2023
Closed
@facebook-github-bot
Copy link
Contributor

@digantdesai merged this pull request in 99f912a.

@digantdesai digantdesai mentioned this pull request Jan 9, 2024
Closed
@guangy10 guangy10 mentioned this pull request Jan 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robell robell robell left review comments

@digantdesai digantdesai digantdesai approved these changes

@cccclai cccclai Awaiting requested review from cccclai

Assignees

@digantdesai digantdesai

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Jerry-Ge @facebook-github-bot @digantdesai @robell