Skip to content

Don't default to -fno-exceptions -fno-rtti #7746

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.

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

Merged
merged 10 commits into from
Jan 28, 2025
Merged

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

@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 <[email protected]>
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