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

Stack from ghstack (oldest at bottom):

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

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

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants