Skip to content

Add more Edge->TOSA lowerings and fixes #475

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 2 commits into from
Closed

Add more Edge->TOSA lowerings and fixes #475

wants to merge 2 commits into from

Conversation

Jerry-Ge
Copy link
Contributor

  • FP32 Softmax lowering
  • Conv2d lowering with groups
  • Add promtote_shape and tranpose helpers for aligning with TOSA rank requirements
  • Add more e2e test cases
  • Other minor fixes

@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 Sep 25, 2023
@netlify
Copy link

netlify bot commented Sep 25, 2023 •
edited
Loading

✅ Deploy Preview for resplendent-gnome-14e531 canceled.

Name Link
🔨 Latest commit f9699e4
🔍 Latest deploy log https://app.netlify.com/sites/resplendent-gnome-14e531/deploys/651245e0eba40f00086eb93a

@digantdesai digantdesai self-requested a review September 25, 2023 16:40
digantdesai
digantdesai requested changes Sep 25, 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 Jerry. The PR is looking good at a high level. I left some comments inline. Getting so close to MV2 :)

Also in the future no need to squash the commits, you may have >1 commits per PR. It is good to keep one logical change per commit.

@Jerry-Ge
Copy link
Contributor Author

Jerry-Ge commented Sep 25, 2023 •
edited
Loading

it's weird that I had no lintrunner issues locally but the CI run failed:
Tried lintrunner innit, still no error locally.
Still don't know why it's passing locally, followed the suggestions from CI failure and pushed in commit#3

lintrunner
Warning: Could not find a lintrunner config at: '.lintrunner.private.toml'. Continuing without using configuration file.
  FLAKE8 success!
  UFMT success!
  CLANGFORMAT success!
  CMAKE success!
ok No lint issues.

cccclai
cccclai reviewed Sep 25, 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.

Looks nice! Really minor comments and they can be follow up pull request.

@Jerry-Ge
Add more Edge->TOSA lowerings and fixes
111b8eb 
* FP32 Softmax lowering
* Conv2d lowering with groups
* Add promtote_shape and tranpose helpers for aligning with TOSA rank
  requirements
* Add more e2e test cases
* Other minor fixes

Signed-off-by: Jerry Ge <[email protected]>
digantdesai
digantdesai approved these changes Sep 26, 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 Jerry. If you think this is good to go and no changes are pending on your side for this PR, I can merge this. Please let me know.

@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 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@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.

@robell
Copy link
Collaborator

robell commented Sep 26, 2023

I found lintrunner didn't cover all files after local commit so use:lintrunner --force-color --all-files to get similar behaviour to CI

kimishpatel
kimishpatel reviewed Sep 26, 2023
View reviewed changes
backends/arm/arm_backend.py
[conv2d_res.name],
attr,
)
if group.number > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

For this to be depthwise conv you need group.number == output_channels == input_channels, right?

kimishpatel
kimishpatel reviewed Sep 26, 2023
View reviewed changes
backends/arm/arm_backend.py
@@ -538,6 +536,79 @@ def preprocess( # noqa: C901
[outp.name],
attr_output_transpose,
)
elif exir_ops.edge.aten._softmax.default == node.target:
Copy link
Contributor

Choose a reason for hiding this comment

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

curious, are you doing fp emulation for softmax?

@facebook-github-bot
Copy link
Contributor

@digantdesai merged this pull request in f6a8d9d.

@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
@Jack-Khuu @malfet
Updating README to mention HF access requests (#475)
9b710b0
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

@cccclai cccclai cccclai left review comments

@kimishpatel kimishpatel kimishpatel left review comments

@digantdesai digantdesai digantdesai approved these changes

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
@Jerry-Ge @facebook-github-bot @robell @digantdesai @cccclai @kimishpatel