Skip to content

[naga] Remove non-essential override references via compaction #7703

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

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

andyleiserson
Copy link
Contributor

@andyleiserson andyleiserson commented May 19, 2025

Adds a mode to compaction that removes unused functions, global variables, and named types and overrides. This mode is used everywhere except the compaction at the end of lowering, where it is important to preserve unused items for type checking and other validation of the module.

Pruning all but the active entry point and then compacting makes process_overrides tolerant of missing values for overrides that are not used by the active entry point.

Fixes #5885

I have opened this as a self-contained pull-request for ease of reviewing, but in order to pass CI, it will need to be rebased on top of the changes in #7711.

Testing
Adds several cases in the validation test to verify that compaction removes references to non-essential overrides.

Squash or Rebase? Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

Adds a mode to compaction that removes unused functions, global
variables, and named types and overrides. This mode is used
everywhere except the compaction at the end of lowering, where
it is important to preserve unused items for type checking and
other validation of the module.

Pruning all but the active entry point and then compacting makes
`process_overrides` tolerant of missing values for overrides that are
not used by the active entry point.

Fixes gfx-rs#5885
@andyleiserson
Copy link
Contributor Author

There is a question what to do about the compact feature. Currently wgsl-in requires compact, so in many environments it is already required. But when wgsl-in is not set, we need a strategy for controlling the use of compaction in process_overrides:

  1. Remove the compact feature and build the associated functionality unconditionally.
  2. Make process_overrides conditional upon the compact feature, and arrange for the feature to be set when necessary. (Currently the no-features clippy runs in CI are broken, because wgpu-hal uses process_overrides, but does not enable compact.)
  3. Make process_overrides available unconditionally, but when built without the compact feature, specifying all overrides is required.
  4. Make process_overrides conditional upon a new feature (overrides?) which would depend on compact.

I have listed in my order of preference, but I don't have all the context about use cases and build size concerns that might justify one of my non-preferred options.

let (module, info) = naga::back::pipeline_constants::process_overrides(
module,
info,
None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already came in useful to be able to pass the actual entry point here, and I have the changes to do that, but unless there's a desire to have them in this PR, I'll hold them for a follow-up so that this one doesn't get any bigger.

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.

Naga shouldn't evaluate overrides unless they're used by the entry point
2 participants