Skip to content

Switch rustc_codegen_spirv to static lib #859

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

Closed
wants to merge 1 commit into from
Closed

Conversation

repi
Copy link
Contributor

@repi repi commented Apr 10, 2022

Was set as a dylib but not really sure why. Having it as a static library enables dependencies, like spriv-builder, to start building directly after the frontend section of the build of the crate is done. Rather than having to wait for the codegen as well.

Saw clean ´spirv-builder` build times go from 46.3 -> 45.8 seconds with this. Have seen bigger benefits when combined with other optimizations as well (example: #858 (comment))

Part of #858

Was set as a `dylib` but not really sure why. Having it as a static library enables dependencies, like `spriv-builder`, to start building directly after the frontend section of the build of the crate is done. Rather than having to wait for the codegen as well.

Saw clean ´spirv-builder` build times go from 46.3 -> 45.8 seconds with this. Have seen bigger benefits when combined with other optimizations as well.
@repi repi requested a review from eddyb as a code owner April 10, 2022 12:14
@repi repi changed the title Switch rustc_codgen_spirv to static lib Switch rustc_codegen_spirv to static lib Apr 10, 2022
@repi
Copy link
Contributor Author

repi commented Apr 10, 2022

Interesting CI failure, so we do need this as dylib for the (doc) tests?

Oh wait doesn't it have to be dylib for rustc to load the codegen plugin dynamically? Now I remember :) Though not sure how it could have worked to build anything for me locally with this then, I did do a cargo clean before hand and examples did work - but maybe it still hadn't properly cleaned out a previously built .dylib and used that still

@repi repi marked this pull request as draft April 10, 2022 14:59
@repi
Copy link
Contributor Author

repi commented Apr 10, 2022

That must have indeed been the case, as can't reproduce this building without being a dylib on a separate machine after properly clean build.

@repi repi closed this Apr 10, 2022
@eddyb
Copy link
Contributor

eddyb commented Apr 11, 2022

I did do a cargo clean before hand and examples did work

Heh, I don't trust tools to do this properly (for whatever reason), and I rm -rf target/ instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants