Skip to content

Add initial lowering of aten.convolution to tosa.conv2d support #615

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

Add initial lowering of aten.convolution to tosa.conv2d support #615

wants to merge 1 commit into from

Conversation

tatwaichong
Copy link
Contributor

This change add quantized int8 support and some test cases.

@netlify
Copy link

netlify bot commented Oct 4, 2023 •
edited
Loading

✅ Deploy Preview for resplendent-gnome-14e531 ready!

Name Link
🔨 Latest commit 06e44f5
🔍 Latest deploy log https://app.netlify.com/sites/resplendent-gnome-14e531/deploys/6521d4b4e4008500093372b9
😎 Deploy Preview https://deploy-preview-615--resplendent-gnome-14e531.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@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 Oct 4, 2023
@tatwaichong tatwaichong force-pushed the tosa_conv2d branch 2 times, most recently from 47549ba to f1a3a1e Compare October 4, 2023 20:42
@Jerry-Ge
Copy link
Contributor

Jerry-Ge commented Oct 4, 2023

Thanks Tatwai for the patch. One question, does the DepthwiseConv2d also work? I didn't see the test cases where the group number is greater 1. The DepthwiseConv2d is heavily used in MobileNetV2.

@Jerry-Ge
Copy link
Contributor

Jerry-Ge commented Oct 4, 2023 •
edited
Loading

@digantdesai @cccclai Please also help review this patch. tks!

@tatwaichong
Copy link
Contributor Author

@Jerry-Ge DepthwiseConv2d support will be in another patch.

cccclai
cccclai reviewed Oct 5, 2023
View reviewed changes
Copy link
Contributor

@cccclai cccclai left a comment

Choose a reason for hiding this comment

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

Thanks for the pr. Just comment some nits. Let's iterate on it

digantdesai
digantdesai requested changes Oct 5, 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.

Thanks for the diff. At a high level it looks good. Let's try to address some comments Chen and I left here.

@tatwaichong
Copy link
Contributor Author

Add non-bias conv support by creating zero tensor.

@digantdesai
Copy link
Contributor

Looks good, let's fix the class counter, and a couple of other nit comments.

digantdesai
digantdesai reviewed Oct 6, 2023
View reviewed changes
backends/arm/arm_backend.py Outdated
elif out in edge_program.graph_signature.inputs_to_buffers:
parameter_name = edge_program.graph_signature.inputs_to_buffers[
node.name
]
p_data = edge_program.state_dict[parameter_name]

assert isinstance(p_data, torch.Tensor), "Expect Attr to be tensor"
weight_values = p_data.detach().numpy()
parameter_values = p_data.detach().numpy()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
parameter_values = p_data.detach().numpy()
buffer_values = p_data.detach().numpy()

digantdesai
digantdesai approved these changes Oct 6, 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 thanks @tatwaichong !

https://github.com/pytorch/executorch/pull/615/files#r1349274983 - can we resolve this before merging?

@tatwaichong
Copy link
Contributor Author

Hi, I responded at the conversation above directly to see if I catch your suggestion.

@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 Jerry-Ge mentioned this pull request Oct 6, 2023
Closed
@tatwaichong
Copy link
Contributor Author

Remove simple_depthwise_conv2d and block_bottleneck_residual from BI-profile unittest list as the quantized depthwise convolution hasn't been supported yet. This removal need the fix from /pull/659, thus copy the needed fix code to this PR for passing the unitest.

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

@facebook-github-bot
Copy link
Contributor

@digantdesai merged this pull request in 51d6afa.

@tatwaichong tatwaichong deleted the tosa_conv2d branch October 9, 2023 07:17
@robell robell added the partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm label Jul 4, 2024
Gasoonjia pushed a commit that referenced this pull request Jul 30, 2024
@malfet
Mitigate ET linker crashes by pining to a specific commit (#615)
fe70ea2 
Picked f0f4db8 and it seems to mitigate the crashes that manifested as follows:
```
   0  0x104ac3648  __assert_rtn + 72
   1  0x1049ebc5c  ld::Fixup::applyFixup(ld::Atom const*, ld::LayoutLinkedImage const&, unsigned char*) const + 8268
   2  0x104a7e7d8  ___ZN2ld16LayoutExecutable27writeContentWithoutLinkEditENSt3__14spanIhLm18446744073709551615EEEy_block_invoke + 332
   3  0x19af0a428  _dispatch_client_callout2 + 20
   4  0x19af1e850  _dispatch_apply_invoke3 + 336
   5  0x19af0a3e8  _dispatch_client_callout + 20
   6  0x19af0bc68  _dispatch_once_callout + 32
   7  0x19af1eeec  _dispatch_apply_invoke_and_wait + 372
   8  0x19af1de9c  _dispatch_apply_with_attr_f + 1212
   9  0x19af1e08c  dispatch_apply + 96
   10  0x104a7e9e4  void mapReduce<ld::Atom const*, mach_o::Error>(std::__1::span<ld::Atom const*, 18446744073709551615ul>, unsigned long, void (unsigned long, mach_o::Error&, std::__1::span<ld::Atom const*, 18446744073709551615ul>) block_pointer, void (std::__1::span<mach_o::Error, 18446744073709551615ul>) block_pointer) + 336
   11  0x104a7e594  ld::LayoutExecutable::writeContentWithoutLinkEdit(std::__1::span<unsigned char, 18446744073709551615ul>, unsigned long long) + 1180
   12  0x104a84020  ld::LayoutExecutable::writeToFile(char const*) + 15248
   13  0x104a362e8  main + 9424
   ld: Assertion failed: (extras.otherInstrOffset != 0 && "Kind::arm64_adrp_ldr missing extra info"), function applyFixup, file Fixup.cpp, line 793.
   clang: error: linker command failed with exit code 1 (use -v to see invocation)
```

TODOS:
 - [ ] Bisect this to a specific change
 - [ ] Check if moving to newer Xcode will work
 - [ ] Write a workflow that auto-updates PT + ET pins
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@digantdesai digantdesai digantdesai approved these changes

@cccclai cccclai cccclai left review comments

@Jerry-Ge Jerry-Ge Jerry-Ge left review comments

Assignees
No one assigned
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tatwaichong @Jerry-Ge @digantdesai @facebook-github-bot @cccclai @robell