Skip to content
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

Add section for cranelift #1523

Merged
merged 10 commits into from
Jul 7, 2024
Merged

Conversation

janhohenheim
Copy link
Member

@janhohenheim janhohenheim commented Jul 4, 2024

Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

This looks very good right now, but one nit I have is to only compile your binary with Cranelift. In the October 2023 progress report, it mentions how you can enable Cranelift for specific packages.

In this case, I think it would be best to recommend to only compile your binary with Cranelift, so that Bevy and the rest still receive better optimizations. This is the same reason why we recommend opt-level = 3 for dependencies.

content/learn/quick-start/getting-started/setup.md Outdated Show resolved Hide resolved
@janhohenheim
Copy link
Member Author

janhohenheim commented Jul 4, 2024

@BD103 that makes sense, thanks for the suggestion!

@janhohenheim
Copy link
Member Author

Blocked by rust-lang/rustc_codegen_cranelift#1504

@janhohenheim
Copy link
Member Author

janhohenheim commented Jul 4, 2024

While we are on the subject, @bjorn3, is there a way to compile my dependencies with LLVM and use cranelift only for my binary on Windows? The way I've set it up locally, I need to run cargo clif manually and I don't know how I would tell it to compile in the specified way. There's no way to do a similar setup to Linux and macOS by just changing one's config.toml on Windows, right?

Also, I heard Bevy stuff on Wasm didn't build or didn't run with cranelift. I don't have the time to verify this. Is this still the case? If so, I can point it out in the readme as well.

@bjorn3
Copy link
Contributor

bjorn3 commented Jul 4, 2024

is there a way to compile my dependencies with LLVM and use cranelift only for my binary on Windows?

Not really. The codegen-backends option in .cargo/config.toml only works for codegen backend shipped with rustc. Rustc itself supports passing the path to a codegen backend dylib (which cargo-clif internally uses), but for cargo I was asked to restrict this unfortunately.

Also, I heard Bevy stuff on Wasm didn't build or didn't run with cranelift. I don't have the time to verify this. Is this still the case? If so, I can point it out in the readme as well.

This is still the case.

@bjorn3
Copy link
Contributor

bjorn3 commented Jul 4, 2024

rust-lang/rust#127177 would also be really nice to land first. This will ship cg_clif for arm64 macOS.

@janhohenheim
Copy link
Member Author

@bjorn3 I've added both links to the PR description for visibility :)

@janhohenheim
Copy link
Member Author

janhohenheim commented Jul 7, 2024

@bjorn3 since we discovered a runtime crash on macOS in the meantime, would it be fine if we changed the section to recommend cranelift specifically for Linux for now? I'd like to have this section merged in time for the Bevy jam.
If that's okay with you, I'll set up a tracking issue for mentioning macOS as well when it's ready.

@bjorn3
Copy link
Contributor

bjorn3 commented Jul 7, 2024

Yeah, that is fine. Maybe also explicitly mention why it shouldn't be used on Windows and macOS to avoid people trying to do it anyway.

@janhohenheim janhohenheim added S-Ready-For-Final-Review Ready for a maintainer to consider for merging and removed S-Needs-Review labels Jul 7, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 7, 2024
Merged via the queue into bevyengine:main with commit 9e63225 Jul 7, 2024
10 checks passed
@janhohenheim janhohenheim deleted the patch-3 branch July 7, 2024 20:34
@janhohenheim janhohenheim mentioned this pull request Jul 7, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants