Skip to content

RFC: Compute only in int32/long/float/double for portable ops to save size#9635

Closed
swolchok wants to merge 2 commits intogh/swolchok/400/headfrom
gh/swolchok/401/head
Closed

RFC: Compute only in int32/long/float/double for portable ops to save size#9635
swolchok wants to merge 2 commits intogh/swolchok/400/headfrom
gh/swolchok/401/head

Conversation

@swolchok
Copy link
Copy Markdown
Contributor

@swolchok swolchok commented Mar 25, 2025

Concern: what if we're running on some sort of 16-bit microcontroller where this is a pessimization?

@swolchok
Update
6b0e11f 
[ghstack-poisoned]
@swolchok
Copy link
Copy Markdown
Contributor Author

swolchok commented Mar 25, 2025 •
edited
Loading

Stack from ghstack (oldest at bottom):

@pytorch-bot
Copy link
Copy Markdown

pytorch-bot Bot commented Mar 25, 2025 •
edited
Loading

🔗 Helpful Links

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

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

❌ 5 New Failures, 11 Pending

As of commit ac64f9e with merge base 811352d (image):

NEW FAILURES - The following jobs have failed:

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

swolchok added a commit that referenced this pull request Mar 25, 2025
@swolchok
RFC: Compute only in int32/long/float/double for portable ops to save…
dea567f 
… size

Concern: what if we're running on some sort of 16-bit microcontroller where this is a pessimization?
ghstack-source-id: 66b89b1
ghstack-comment-id: 2752794343
Pull-Request-resolved: #9635
@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 Mar 25, 2025
This was referenced Mar 25, 2025
Merged
Merged
Closed
@swolchok
Copy link
Copy Markdown
Contributor Author

swolchok commented Mar 25, 2025

some sort of 16-bit microcontroller

wasn't sure if I was making things up, so: https://developer.arm.com/Processors/Ethos-U55 is a real example from the present day

@swolchok
Copy link
Copy Markdown
Contributor Author

swolchok commented Mar 25, 2025

size impact: on my mac, test/build_size_test.sh reports that size_test_all_ops has size 1205136 before this PR and 1105856 after, a decrease of around 8%

@swolchok
Copy link
Copy Markdown
Contributor Author

swolchok commented Mar 26, 2025 •
edited
Loading

This is known to break tests, I think because it breaks SupportedTensorDtypes::SAME_AS_COMMON for reasons outlined in #9613, hence it is RFC status. The problem is fixable, but if we have directional concerns with this then I don't want to invest in fixing it.

@swolchok
Update
ac64f9e 
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 26, 2025
@swolchok
RFC: Compute only in int32/long/float/double for portable ops to save…
da4308f 
… size

Concern: what if we're running on some sort of 16-bit microcontroller where this is a pessimization?
ghstack-source-id: a91380c
ghstack-comment-id: 2752794343
Pull-Request-resolved: #9635
@swolchok
Copy link
Copy Markdown
Contributor Author

swolchok commented Mar 26, 2025 •
edited
Loading

per discussion with @manuelcandales, if we do this then we we need to cast through the "actual" compute type before casting to the output type so that we match ATen. example:

torch.ops.aten.mul(torch.tensor([100], dtype=torch.int8), torch.tensor([100], dtype=torch.int8), out=torch.zeros([1], dtype=torch.long))
tensor([16])

computing in int32 or int16 would cause this to yield 10000, not 16; casting through int8 would correct this.

@swolchok
Copy link
Copy Markdown
Contributor Author

swolchok commented Mar 26, 2025

This is a bad idea because smaller compute dtypes benefit from additional SIMD lanes.

@swolchok swolchok closed this Mar 26, 2025
digantdesai
digantdesai reviewed Mar 27, 2025
View reviewed changes
Comment thread kernels/portable/cpu/util/elementwise_util.h
// Gate above optimization off if we appear to be on some kind of 8-bit or
// 16-bit CPU, which would invalidate our assumption about 32-bit
// math being just as fast.
constexpr bool cpu_appears_to_be_at_least_32_bit = sizeof(void*) >= 4 && sizeof(int) >= 4;
Copy link
Copy Markdown
Contributor

@digantdesai digantdesai Mar 27, 2025

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@digantdesai digantdesai digantdesai left review comments

@manuelcandales manuelcandales Awaiting requested review from manuelcandales manuelcandales is a code owner

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@swolchok @digantdesai @facebook-github-bot