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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ jobs:

# Check with all compatible features
cargo clippy --target ${{ matrix.target }} ${{ matrix.extra-flags }} -p wgpu-types --no-default-features --features strict_asserts,fragile-send-sync-non-atomic-wasm,serde,counters
cargo clippy --target ${{ matrix.target }} ${{ matrix.extra-flags }} -p naga --no-default-features --features dot-out,compact
cargo clippy --target ${{ matrix.target }} ${{ matrix.extra-flags }} -p naga --no-default-features --features dot-out
cargo clippy --target ${{ matrix.target }} ${{ matrix.extra-flags }} -p wgpu-hal --no-default-features --features fragile-send-sync-non-atomic-wasm

# Building for native platforms with standard tests.
Expand Down
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions cts_runner/test.lst
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ webgpu:api,operation,command_buffer,copyBufferToBuffer:state_transitions:*
webgpu:api,operation,command_buffer,copyBufferToBuffer:copy_order:*
webgpu:api,operation,compute,basic:memcpy:*
//FAIL: webgpu:api,operation,compute,basic:large_dispatch:*
webgpu:api,operation,compute_pipeline,overrides:*
webgpu:api,operation,device,lost:*
webgpu:api,operation,render_pipeline,overrides:*
webgpu:api,operation,rendering,basic:clear:*
webgpu:api,operation,rendering,basic:fullscreen_quad:*
//FAIL: webgpu:api,operation,rendering,basic:large_draw:*
Expand Down
1 change: 0 additions & 1 deletion naga-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ test = false

[dependencies]
naga = { workspace = true, features = [
"compact",
"wgsl-in",
"wgsl-out",
"glsl-in",
Expand Down
42 changes: 29 additions & 13 deletions naga-cli/src/bin/naga.rs
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ fn run() -> anyhow::Result<()> {
write_output(&module, &info, &params, before_compaction)?;
}

naga::compact::compact(&mut module);
naga::compact::compact(&mut module, false);

// Re-validate the IR after compaction.
match naga::valid::Validator::new(params.validation_flags, validation_caps)
Expand Down Expand Up @@ -717,9 +717,13 @@ fn write_output(
succeed, and it failed in a previous step",
))?;

let (module, info) =
naga::back::pipeline_constants::process_overrides(module, info, &params.overrides)
.unwrap_pretty();
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.

&params.overrides,
)
.unwrap_pretty();

let pipeline_options = msl::PipelineOptions::default();
let (msl, _) =
Expand Down Expand Up @@ -751,9 +755,13 @@ fn write_output(
succeed, and it failed in a previous step",
))?;

let (module, info) =
naga::back::pipeline_constants::process_overrides(module, info, &params.overrides)
.unwrap_pretty();
let (module, info) = naga::back::pipeline_constants::process_overrides(
module,
info,
None,
&params.overrides,
)
.unwrap_pretty();

let spv =
spv::write_vec(&module, &info, &params.spv_out, pipeline_options).unwrap_pretty();
Expand Down Expand Up @@ -788,9 +796,13 @@ fn write_output(
succeed, and it failed in a previous step",
))?;

let (module, info) =
naga::back::pipeline_constants::process_overrides(module, info, &params.overrides)
.unwrap_pretty();
let (module, info) = naga::back::pipeline_constants::process_overrides(
module,
info,
None,
&params.overrides,
)
.unwrap_pretty();

let mut buffer = String::new();
let mut writer = glsl::Writer::new(
Expand Down Expand Up @@ -819,9 +831,13 @@ fn write_output(
succeed, and it failed in a previous step",
))?;

let (module, info) =
naga::back::pipeline_constants::process_overrides(module, info, &params.overrides)
.unwrap_pretty();
let (module, info) = naga::back::pipeline_constants::process_overrides(
module,
info,
None,
&params.overrides,
)
.unwrap_pretty();

let mut buffer = String::new();
let pipeline_options = Default::default();
Expand Down
5 changes: 2 additions & 3 deletions naga/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ arbitrary = [
]
spv-in = ["dep:petgraph", "petgraph/graphmap", "dep:spirv"]
spv-out = ["dep:spirv"]
wgsl-in = ["dep:hexf-parse", "dep:strum", "dep:unicode-ident", "compact"]
wgsl-in = ["dep:hexf-parse", "dep:strum", "dep:unicode-ident"]
wgsl-out = []

## Enables outputting to HLSL (Microsoft's High-Level Shader Language).
Expand All @@ -72,8 +72,6 @@ hlsl-out = []
## If you want to enable HLSL output it regardless of the target platform, use `naga/hlsl-out`.
hlsl-out-if-target-windows = []

compact = []

## Enables colored output through codespan-reporting and termcolor.
termcolor = ["codespan-reporting/termcolor"]

Expand All @@ -85,6 +83,7 @@ arbitrary = { workspace = true, features = ["derive"], optional = true }
arrayvec.workspace = true
bitflags.workspace = true
bit-set.workspace = true
bit-vec.workspace = true
cfg-if.workspace = true
codespan-reporting = { workspace = true }
hashbrown.workspace = true
Expand Down
27 changes: 27 additions & 0 deletions naga/src/arena/handle_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ impl<T> HandleSet<T> {
}
}

/// Add all of the handles that can be included in this set.
pub fn add_all(&mut self) {
self.members.get_mut().set_all();
}

pub fn contains(&self, handle: Handle<T>) -> bool {
self.members.contains(handle.index())
}
Expand All @@ -85,6 +90,28 @@ impl<T> HandleSet<T> {
pub fn iter(&self) -> impl '_ + Iterator<Item = Handle<T>> {
self.members.iter().map(Handle::from_usize)
}

/// Removes and returns the numerically largest handle in the set, or `None`
/// if the set is empty.
pub fn pop(&mut self) -> Option<Handle<T>> {
let members = core::mem::take(&mut self.members);
let mut vec = members.into_bit_vec();
let result = vec
.iter_mut()
.enumerate()
.rev()
.filter_map(|(i, mut bit)| {
if *bit {
*bit = false;
Some(i)
} else {
None
}
})
.next();
self.members = bit_set::BitSet::from_bit_vec(vec);
result.map(Handle::from_usize)
}
}

impl<T> Default for HandleSet<T> {
Expand Down
1 change: 0 additions & 1 deletion naga/src/arena/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,6 @@ impl<T> Arena<T> {
Ok(())
}

#[cfg(feature = "compact")]
pub(crate) fn retain_mut<P>(&mut self, mut predicate: P)
where
P: FnMut(Handle<T>, &mut T) -> bool,
Expand Down
3 changes: 0 additions & 3 deletions naga/src/arena/unique_arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ impl<T> UniqueArena<T> {
.unwrap_or(&Span::default())
}

#[cfg(feature = "compact")]
pub(crate) fn drain_all(&mut self) -> UniqueArenaDrain<T> {
UniqueArenaDrain {
inner_elts: self.set.drain(..),
Expand All @@ -82,14 +81,12 @@ impl<T> UniqueArena<T> {
}
}

#[cfg(feature = "compact")]
pub struct UniqueArenaDrain<'a, T> {
inner_elts: indexmap::set::Drain<'a, T>,
inner_spans: alloc::vec::Drain<'a, Span>,
index: Index,
}

#[cfg(feature = "compact")]
impl<T> Iterator for UniqueArenaDrain<'_, T> {
type Item = (Handle<T>, T, Span);

Expand Down
13 changes: 13 additions & 0 deletions naga/src/back/pipeline_constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ use thiserror::Error;
use super::PipelineConstants;
use crate::{
arena::HandleVec,
compact::compact,
ir,
proc::{ConstantEvaluator, ConstantEvaluatorError, Emitter},
valid::{Capabilities, ModuleInfo, ValidationError, ValidationFlags, Validator},
Arena, Block, Constant, Expression, Function, Handle, Literal, Module, Override, Range, Scalar,
Expand Down Expand Up @@ -55,6 +57,7 @@ pub enum PipelineConstantError {
pub fn process_overrides<'a>(
module: &'a Module,
module_info: &'a ModuleInfo,
entry_point: Option<(ir::ShaderStage, &str)>,
pipeline_constants: &PipelineConstants,
) -> Result<(Cow<'a, Module>, Cow<'a, ModuleInfo>), PipelineConstantError> {
if module.overrides.is_empty() {
Expand All @@ -63,6 +66,16 @@ pub fn process_overrides<'a>(

let mut module = module.clone();

// If an entry point was specified, compact the module to remove anything
// not reachable from that entry point. This is necessary because we may not
// have values for overrides that are not reachable from the entry point.
if let Some((ep_stage, ep_name)) = entry_point {
module
.entry_points
.retain(|ep| ep.stage == ep_stage && ep.name == ep_name);
}
compact(&mut module, false);

// A map from override handles to the handles of the constants
// we've replaced them with.
let mut override_map = HandleVec::with_capacity(module.overrides.len());
Expand Down
18 changes: 14 additions & 4 deletions naga/src/compact/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ pub struct ExpressionTracer<'tracer> {
/// The used map for `types`.
pub types_used: &'tracer mut HandleSet<crate::Type>,

/// The used map for global variables.
pub global_variables_used: &'tracer mut HandleSet<crate::GlobalVariable>,

/// The used map for `constants`.
pub constants_used: &'tracer mut HandleSet<crate::Constant>,

Expand Down Expand Up @@ -76,9 +79,7 @@ impl ExpressionTracer<'_> {
// Expressions that do not contain handles that need to be traced.
Ex::Literal(_)
| Ex::FunctionArgument(_)
| Ex::GlobalVariable(_)
| Ex::LocalVariable(_)
| Ex::CallResult(_)
| Ex::SubgroupBallotResult
| Ex::RayQueryProceedResult => {}

Expand Down Expand Up @@ -134,6 +135,9 @@ impl ExpressionTracer<'_> {
} => {
self.expressions_used.insert(vector);
}
Ex::GlobalVariable(handle) => {
self.global_variables_used.insert(handle);
}
Ex::Load { pointer } => {
self.expressions_used.insert(pointer);
}
Expand Down Expand Up @@ -233,6 +237,10 @@ impl ExpressionTracer<'_> {
Ex::ArrayLength(expr) => {
self.expressions_used.insert(expr);
}
// CallResult expressions do contain a function handle, but any used
// CallResult expression should have an associated `ir::Statement::Call`
// that we will trace.
Ex::CallResult(_) => {}
Ex::AtomicResult { ty, comparison: _ }
| Ex::WorkGroupUniformLoadResult { ty }
| Ex::SubgroupOperationResult { ty } => {
Expand Down Expand Up @@ -267,9 +275,7 @@ impl ModuleMap {
// Expressions that do not contain handles that need to be adjusted.
Ex::Literal(_)
| Ex::FunctionArgument(_)
| Ex::GlobalVariable(_)
| Ex::LocalVariable(_)
| Ex::CallResult(_)
| Ex::SubgroupBallotResult
| Ex::RayQueryProceedResult => {}

Expand Down Expand Up @@ -306,6 +312,7 @@ impl ModuleMap {
ref mut vector,
pattern: _,
} => adjust(vector),
Ex::GlobalVariable(ref mut handle) => self.globals.adjust(handle),
Ex::Load { ref mut pointer } => adjust(pointer),
Ex::ImageSample {
ref mut image,
Expand Down Expand Up @@ -392,6 +399,9 @@ impl ModuleMap {
kind: _,
convert: _,
} => adjust(expr),
Ex::CallResult(ref mut function) => {
self.functions.adjust(function);
}
Ex::AtomicResult {
ref mut ty,
comparison: _,
Expand Down
13 changes: 12 additions & 1 deletion naga/src/compact/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ pub struct FunctionTracer<'a> {
pub constants: &'a crate::Arena<crate::Constant>,
pub overrides: &'a crate::Arena<crate::Override>,

pub functions_pending: &'a mut HandleSet<crate::Function>,
pub functions_used: &'a mut HandleSet<crate::Function>,
pub types_used: &'a mut HandleSet<crate::Type>,
pub global_variables_used: &'a mut HandleSet<crate::GlobalVariable>,
pub constants_used: &'a mut HandleSet<crate::Constant>,
pub overrides_used: &'a mut HandleSet<crate::Override>,
pub global_expressions_used: &'a mut HandleSet<crate::Expression>,
Expand All @@ -16,6 +19,13 @@ pub struct FunctionTracer<'a> {
}

impl FunctionTracer<'_> {
pub fn trace_call(&mut self, function: crate::Handle<crate::Function>) {
if !self.functions_used.contains(function) {
self.functions_used.insert(function);
self.functions_pending.insert(function);
}
}

pub fn trace(&mut self) {
for argument in self.function.arguments.iter() {
self.types_used.insert(argument.ty);
Expand Down Expand Up @@ -53,6 +63,7 @@ impl FunctionTracer<'_> {
expressions: &self.function.expressions,

types_used: self.types_used,
global_variables_used: self.global_variables_used,
constants_used: self.constants_used,
overrides_used: self.overrides_used,
expressions_used: &mut self.expressions_used,
Expand Down Expand Up @@ -105,6 +116,6 @@ impl FunctionMap {
assert!(reuse.is_empty());

// Adjust statements.
self.adjust_body(function);
self.adjust_body(function, &module_map.functions);
}
}
Loading
Loading