Skip to content

Conversation

@swolchok
Copy link
Contributor

We need exceptions/RTTI for non-core-only use cases (see #7736).
Sending first version of this without patching any specific targets to restore these flags because I want to see if anything actually breaks.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@swolchok
Copy link
Contributor Author

swolchok commented Jan 17, 2025

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 17, 2025

🔗 Helpful Links

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

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

✅ No Failures

As of commit 137da0f with merge base 7bc06d1 (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 Jan 17, 2025
swolchok added a commit that referenced this pull request Jan 17, 2025
We need exceptions/RTTI for non-core-only use cases (see #7736). Sending first version of this without patching any specific targets to restore these flags because I want to see if anything actually breaks. ghstack-source-id: c8fbe3a ghstack-comment-id: 2599307626 Pull Request resolved: #7746
@swolchok
Copy link
Contributor Author

phi-3-mini and emformer-transcribe failures are from trunk, so it looks likely that this doesn't break anything.

@swolchok
Copy link
Contributor Author

Specific question for reviewers: which CI builds, if any, do we want/need to disable exceptions/rtti? I could go either way on the size test; for the case where we include non-portable ops we need to leave them enabled, but it's up to us what we want to do for the core-only case.

[ghstack-poisoned]
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 23, 2025
We need exceptions/RTTI for non-core-only use cases (see #7736). Sending first version of this without patching any specific targets to restore these flags because I want to see if anything actually breaks. ghstack-source-id: 9f14a3d ghstack-comment-id: 2599307626 Pull Request resolved: #7746
@mergennachin
Copy link
Contributor

@mergennachin
Copy link
Contributor

@mcremon-meta - are there places in cadence code where we rely on these flags to be present? if so, you might need to change your config explicitly

@mergennachin
Copy link
Contributor

@robell , @freddan80 - FYI, see #7736 for background and context

@swolchok
Copy link
Contributor Author

https://github.com/pytorch/executorch/actions/runs/12919450651/job/36029994766?pr=7746 -- interesting, it is still under the limit

#7742 lowered it from 51768 to 47664, probably because it caused us to -DNDEBUG in release builds. Mildly surprised we are only up to 47672 after this diff; I wonder why these flags were so ineffective at reducing size.

@mcremon-meta
Copy link
Contributor

@mcremon-meta - are there places in cadence code where we rely on these flags to be present? if so, you might need to change your config explicitly

cc @hsharma35 @zonglinpeng in case we need to adjust something

[ghstack-poisoned]
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 23, 2025
We need exceptions/RTTI for non-core-only use cases (see #7736). Sending first version of this without patching any specific targets to restore these flags because I want to see if anything actually breaks. ghstack-source-id: 15560f2 ghstack-comment-id: 2599307626 Pull Request resolved: #7746
@swolchok swolchok added the release notes: build Changes related to build, including dependency upgrades, build flags, optimizations, etc. label Jan 23, 2025
Base automatically changed from gh/swolchok/142/head to main January 23, 2025 23:35
@swolchok
Copy link
Contributor Author

perhaps add a no-except, no-rtti flavor as well.

patched the build script in the latest update.

Can you adjust the current CI test for size_test and size_test_all_ops to use --no-exceptions, --no-rtti flags.

done! here is the effect. before restoring -fno-exceptions -fno-rtti:

ExecuTorch with no ops binary size, unstripped: -rwxr-xr-x 1 swolchok staff 87192 Jan 27 12:12 cmake-out/test/size_test ExecuTorch with portable ops binary size, unstripped: -rwxr-xr-x 1 swolchok staff 1373168 Jan 27 12:12 cmake-out/test/size_test_all_ops stripped: ~/src/executorch> ls -al cmake-out/test/size_test -rwxr-xr-x 1 swolchok staff 73672 Jan 27 12:39 cmake-out/test/size_test ~/src/executorch> ls -al cmake-out/test/size_test_all_ops -rwxr-xr-x 1 swolchok staff 1172688 Jan 27 12:39 cmake-out/test/size_test_all_ops 

after:

ExecuTorch with no ops binary size, unstripped: -rwxr-xr-x 1 swolchok staff 68584 Jan 27 12:10 cmake-out/test/size_test ExecuTorch with portable ops binary size, unstripped: -rwxr-xr-x 1 swolchok staff 1370064 Jan 27 12:10 cmake-out/test/size_test_all_ops stripped: ~/src/executorch> ls -al cmake-out/test/size_test -rwxr-xr-x 1 swolchok staff 56552 Jan 27 12:41 cmake-out/test/size_test ~/src/executorch> ls -al cmake-out/test/size_test_all_ops -rwxr-xr-x 1 swolchok staff 1172064 Jan 27 12:41 cmake-out/test/size_test_all_ops 

I am surprised that core is affected so much more than core + portable ops.

@swolchok
Copy link
Contributor Author

swolchok commented Jan 27, 2025

should probably also patch the cadence build scripts to set -fno-exceptions -fno-rtti so as to preserve the example being good (EDIT: done)

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 28, 2025
We need exceptions/RTTI for non-core-only use cases (see #7736). Sending first version of this without patching any specific targets to restore these flags because I want to see if anything actually breaks. ghstack-source-id: 2aa54d9 ghstack-comment-id: 2599307626 Pull Request resolved: #7746
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 28, 2025
We need exceptions/RTTI for non-core-only use cases (see #7736). Sending first version of this without patching any specific targets to restore these flags because I want to see if anything actually breaks. ghstack-source-id: 91d8ab8 ghstack-comment-id: 2599307626 Pull Request resolved: #7746
@swolchok swolchok merged commit f9aa6c1 into main Jan 28, 2025
108 checks passed
@swolchok swolchok deleted the gh/swolchok/143/head branch January 28, 2025 17:34
YIWENX14 pushed a commit that referenced this pull request Jan 28, 2025
We need exceptions/RTTI for non-core-only use cases (see #7736).
swolchok added a commit that referenced this pull request Jan 30, 2025
… ExecuTorch" As of #7746, we build with exceptions by default, so we just need to use them. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 30, 2025
 As of #7746, we build with exceptions by default, so we just need to use them. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 30, 2025
… ExecuTorch" As of #7746, we build with exceptions by default, so we just need to use them. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 30, 2025
 As of #7746, we build with exceptions by default, so we just need to use them. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
zonglinpeng pushed a commit to zonglinpeng/executorch that referenced this pull request Jan 30, 2025
We need exceptions/RTTI for non-core-only use cases (see pytorch#7736).
swolchok added a commit that referenced this pull request Jan 30, 2025
… ExecuTorch" As of #7746, we build with exceptions by default, so we just need to use them. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 30, 2025
 As of #7746, we build with exceptions by default, so we just need to use them. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 4, 2025
… ExecuTorch" As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 4, 2025
 As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 4, 2025
… ExecuTorch" As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 4, 2025
 As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 5, 2025
… ExecuTorch" As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 5, 2025
 As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 6, 2025
… ExecuTorch" As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 6, 2025
 As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 7, 2025
… ExecuTorch" As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 7, 2025
 As of #7746, we build with exceptions by default, so we just need to use them. Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
swolchok pushed a commit that referenced this pull request Feb 11, 2025
Pull Request resolved: #7546 As of #7746, we build with exceptions by default, so we just need to use them. ghstack-source-id: 265190625 @exported-using-ghexport Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) --------- Co-authored-by: Github Executorch <github_executorch@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: build Changes related to build, including dependency upgrades, build flags, optimizations, etc.

7 participants