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

Implement module compaction #2472

Merged
merged 6 commits into from
Sep 20, 2023
Merged

Implement module compaction #2472

merged 6 commits into from
Sep 20, 2023

Conversation

jimblandy
Copy link
Member

Add a new Naga feature, "compact", which adds a new function naga::compact::compact, which removes unused expressions, types, and constants from a Module.

@jimblandy jimblandy requested a review from teoxoy September 12, 2023 21:47
@jimblandy
Copy link
Member Author

This should be easier to review commit by commit.

This is a step towards having the output loop depend only on `params`,
rather than both `params` and `args`.
Change `--generate-debug-symbols` from an option that requires a
value, "true" or "false", to a switch, whose mere presence enables the
feature.
This lets us gather up the code that influences SPV input.
Add a new Naga feature, `"compact"`, which adds a new function
`naga::compact::compact`, which removes unused expressions, types, and
constants from a `Module`.
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

This is some nice machinery!

I was wondering if we can move the type/expression tracing in the analyzer so that we can always generate the usage metadata.
The backends will then use the metadata for naming expressions and (unnamed) types instead of having to mutate the IR.
Can be done later but do let me know what you think!

@jimblandy
Copy link
Member Author

I was wondering if we can move the type/expression tracing in the analyzer so that we can always generate the usage metadata. The backends will then use the metadata for naming expressions and (unnamed) types instead of having to mutate the IR.

Oh, in the sense that the relocated handle indices are actually the names we want to use in output, so we can skip the compaction step. That makes sense.

I think there's some advantage to mutating the IR, though. Ideally the tests should use the backend code exactly the same way applications do. Mutation lets the backends remain ignorant of whether we performed compaction or not.

I think there are ideas to pursue here, but let's land this as-is for now.

@teoxoy
Copy link
Member

teoxoy commented Sep 20, 2023

Ideally the tests should use the backend code exactly the same way applications do.

I was proposing to always (without it being gated behind a feature flag) create the metadata and use it in the backends.
I was assuming the perf impact would be negligent but I guess we won't know unless we try.

@teoxoy teoxoy merged commit 8b26721 into gfx-rs:master Sep 20, 2023
@jimblandy jimblandy deleted the compact branch September 20, 2023 18:16
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