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

Remove different OpNames that target the same ID #398

Merged
merged 1 commit into from
Jan 28, 2021
Merged

Conversation

khyperia
Copy link
Contributor

There's a bit of unfortunate interaction in the way we link binaries. Here's an order of operations:

  1. We emit OpNames for everything, to make it easier for us to look at binaries and debug them.
  2. We link the module, merging identical types etc. into the same construct. Relevant OpNames get rewritten to point at the same construct.
  3. We strip dead code, types, etc.

This becomes a problem: things like ZSTs and other extremely common types are emitted a lot, with many different names. They're all merged in step 2, but then we have an absolute heap of names all pointing at the same ID, and that ID happens to be used in a few places, so we barf gazillions of strings into the binary.

So, only keep the first name we encounter for any particular ID. This will likely be the "incorrect" name (probably a name that's actually never used and would have been DCE stripped if we never merged identical constructs), but oh well, that name does correspond to the type, close enough.

This reduces ark's spir-v binary size from 743KiB to 93KiB

@khyperia khyperia added the c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. label Jan 28, 2021
@khyperia khyperia requested a review from eddyb as a code owner January 28, 2021 12:16
@eddyb
Copy link
Contributor

eddyb commented Jan 28, 2021

Should we be deduplicating so aggressively? Wouldn't it make more sense to only deduplicate types that connect imports and exports (and therefore must match)?

@khyperia
Copy link
Contributor Author

yeah, probably! that gets way more complicated, though, especially because various things in SPIR-V allow duplication, and various other things don't (e.g. you can only have one OpTypeInt of a certain size, but you can have multiple OpTypeStructs that are the same). It's a whole mess that isn't super worth it (especially since even the basic system we have now has known bugs in it).

@mergify mergify bot merged commit 9f6c91c into main Jan 28, 2021
@mergify mergify bot deleted the duplicate-opname branch January 28, 2021 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants