Skip to content

Commit

Permalink
Allow unconsumed inputs in fragment shaders by removing them from vertex
Browse files Browse the repository at this point in the history
outputs when generating HLSL.

Fixes #3748

* Add naga::back::hlsl::FragmentEntryPoint for providing information
  about the fragment entry point when generating vertex entry points via
  naga::back::hlsl::Writer::write. Vertex outputs not consumed by the
  fragment entry point are omitted in the final output struct.
* Add naga snapshot test for this new feature,
* Remove Features::SHADER_UNUSED_VERTEX_OUTPUT,
  StageError::InputNotConsumed, and associated validation logic.
* Make wgpu dx12 backend pass fragment shader info when generating
  vertex HLSL.
* Add wgpu regression test for allowing unconsumed inputs.
  • Loading branch information
Imberflur committed Apr 14, 2024
1 parent 6756601 commit 1ed945e
Show file tree
Hide file tree
Showing 24 changed files with 364 additions and 67 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ Bottom level categories:
- `wgpu::Texture::as_hal` now returns a user-defined type to match the other as_hal functions

- Added support for pipeline-overridable constants. By @teoxoy & @jimblandy in [#5500](https://github.com/gfx-rs/wgpu/pull/5500)
- Unconsumed vertex outputs are now always allowed. Removed `StageError::InputNotConsumed`, `Features::SHADER_UNUSED_VERTEX_OUTPUT`, and associated validation. By @Imberflur in [#5531](https://github.com/gfx-rs/wgpu/pull/5531)

#### GLES

Expand All @@ -129,6 +130,13 @@ Bottom level categories:

- Allow user to select which MSL version to use via `--metal-version` with Naga CLI. By @pcleavelin in [#5392](https://github.com/gfx-rs/wgpu/pull/5392)
- Support `arrayLength` for runtime-sized arrays inside binding arrays (for WGSL input and SPIR-V output). By @kvark in [#5428](https://github.com/gfx-rs/wgpu/pull/5428)
- In hlsl-out, allow passing information about the fragment entry point to omit vertex outputs that are not in the fragment inputs. By @Imberflur in[#5531](https://github.com/gfx-rs/wgpu/pull/5531)

```diff
let writer: naga::back::hlsl::Writer = /* ... */;
-writer.write(&module, &module_info);
+writer.write(&module, &module_info, None);
```

#### WebGPU

Expand Down
7 changes: 0 additions & 7 deletions deno_webgpu/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,9 +360,6 @@ fn deserialize_features(features: &wgpu_types::Features) -> Vec<&'static str> {
if features.contains(wgpu_types::Features::SHADER_EARLY_DEPTH_TEST) {
return_features.push("shader-early-depth-test");
}
if features.contains(wgpu_types::Features::SHADER_UNUSED_VERTEX_OUTPUT) {
return_features.push("shader-unused-vertex-output");
}

return_features
}
Expand Down Expand Up @@ -648,10 +645,6 @@ impl From<GpuRequiredFeatures> for wgpu_types::Features {
wgpu_types::Features::SHADER_EARLY_DEPTH_TEST,
required_features.0.contains("shader-early-depth-test"),
);
features.set(
wgpu_types::Features::SHADER_UNUSED_VERTEX_OUTPUT,
required_features.0.contains("shader-unused-vertex-output"),
);

features
}
Expand Down
2 changes: 1 addition & 1 deletion naga-cli/src/bin/naga.rs
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,7 @@ fn write_output(

let mut buffer = String::new();
let mut writer = hlsl::Writer::new(&mut buffer, &params.hlsl);
writer.write(&module, &info).unwrap_pretty();
writer.write(&module, &info, None).unwrap_pretty();
fs::write(output_path, buffer)?;
}
"wgsl" => {
Expand Down
29 changes: 29 additions & 0 deletions naga/src/back/hlsl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,35 @@ impl Wrapped {
}
}

/// A fragment entry point to be considered when generating HLSL for the output interface of vertex
/// entry points.
///
/// This is provided as an optional paramter to [`Writer::write`].

Check warning on line 292 in naga/src/back/hlsl/mod.rs

View workflow job for this annotation

GitHub Actions / Format & Typos

"paramter" should be "parameter".
///
/// If this is provided, vertex outputs will be removed if they are not inputs of this fragment
/// entry point. This is necessary for generating correct HLSL when some of the vertex shader
/// outputs are not consumed by the fragment shader.
pub struct FragmentEntryPoint<'a> {
module: &'a crate::Module,
func: &'a crate::Function,
}

impl<'a> FragmentEntryPoint<'a> {
/// Returns `None` if the entry point with the provided name can't be found or isn't a fragment
/// entry point.
pub fn new(module: &'a crate::Module, ep_name: &'a str) -> Option<Self> {
module
.entry_points
.iter()
.find(|ep| ep.name == ep_name)
.filter(|ep| ep.stage == crate::ShaderStage::Fragment)
.map(|ep| Self {
module,
func: &ep.function,
})
}
}

pub struct Writer<'a, W> {
out: W,
names: crate::FastHashMap<proc::NameKey, String>,
Expand Down
72 changes: 65 additions & 7 deletions naga/src/back/hlsl/writer.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::{
help::{WrappedArrayLength, WrappedConstructor, WrappedImageQuery, WrappedStructMatrixAccess},
storage::StoreValue,
BackendResult, Error, Options,
BackendResult, Error, FragmentEntryPoint, Options,
};
use crate::{
back,
Expand All @@ -25,6 +25,7 @@ pub(crate) const INSERT_BITS_FUNCTION: &str = "naga_insertBits";
struct EpStructMember {
name: String,
ty: Handle<crate::Type>,
// TODO: log error if binding is none?
// technically, this should always be `Some`
binding: Option<crate::Binding>,
index: u32,
Expand Down Expand Up @@ -167,6 +168,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
&mut self,
module: &Module,
module_info: &valid::ModuleInfo,
fragment_entry_point: Option<&FragmentEntryPoint<'_>>,
) -> Result<super::ReflectionInfo, Error> {
if !module.overrides.is_empty() {
return Err(Error::Override);
Expand Down Expand Up @@ -266,7 +268,13 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
// Write all entry points wrapped structs
for (index, ep) in module.entry_points.iter().enumerate() {
let ep_name = self.names[&NameKey::EntryPoint(index as u16)].clone();
let ep_io = self.write_ep_interface(module, &ep.function, ep.stage, &ep_name)?;
let ep_io = self.write_ep_interface(
module,
&ep.function,
ep.stage,
&ep_name,
fragment_entry_point,
)?;
self.entry_point_io.push(ep_io);
}

Expand Down Expand Up @@ -460,6 +468,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
writeln!(self.out, "}};")?;
writeln!(self.out)?;

// See ordering notes on EntryPointInterface fields
match shader_stage.1 {
Io::Input => {
// bring back the original order
Expand Down Expand Up @@ -493,6 +502,8 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
for arg in func.arguments.iter() {
match module.types[arg.ty].inner {
TypeInner::Struct { ref members, .. } => {
// TODO: what about nested structs? Is that possible? Maybe try an unwrap on
// the binding?
for member in members.iter() {
let name = self.namer.call_or(&member.name, "member");
let index = fake_members.len() as u32;
Expand Down Expand Up @@ -529,10 +540,10 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
result: &crate::FunctionResult,
stage: ShaderStage,
entry_point_name: &str,
frag_ep: Option<&FragmentEntryPoint<'_>>,
) -> Result<EntryPointBinding, Error> {
let struct_name = format!("{stage:?}Output_{entry_point_name}");

let mut fake_members = Vec::new();
let empty = [];
let members = match module.types[result.ty].inner {
TypeInner::Struct { ref members, .. } => members,
Expand All @@ -542,14 +553,60 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
}
};

for member in members.iter() {
// Gather list of fragment input locations. We use this below to remove user-defined
// varyings from VS outputs that aren't in the FS inputs. This makes the VS interface match
// as long as the FS inputs are a subset of the VS outputs. This is only applied if the
// writer is supplied with information about the fragment entry point.
let fs_input_locs = if let (Some(frag_ep), ShaderStage::Vertex) = (frag_ep, stage) {
let mut fs_input_locs = Vec::new();
for arg in frag_ep.func.arguments.iter() {
let mut push_if_location = |binding: &Option<crate::Binding>| {
match *binding {
Some(crate::Binding::Location { location, .. }) => {
fs_input_locs.push(location)
}
Some(crate::Binding::BuiltIn(_)) => {}
// Log error?
None => {}
}
};
match frag_ep.module.types[arg.ty].inner {
TypeInner::Struct { ref members, .. } => {
// TODO: nesting?
for member in members.iter() {
push_if_location(&member.binding);
}
}
_ => push_if_location(&arg.binding),
}
}
fs_input_locs.sort();
Some(fs_input_locs)
} else {
None
};

let mut fake_members = Vec::new();
for (index, member) in members.iter().enumerate() {
if let Some(ref fs_input_locs) = fs_input_locs {
match member.binding {
Some(crate::Binding::Location { location, .. }) => {
if fs_input_locs.binary_search(&location).is_err() {
continue;
}
}
Some(crate::Binding::BuiltIn(_)) => {}
// Log error?
None => {}
}
}

let member_name = self.namer.call_or(&member.name, "member");
let index = fake_members.len() as u32;
fake_members.push(EpStructMember {
name: member_name,
ty: member.ty,
binding: member.binding.clone(),
index,
index: index as u32,
});
}

Expand All @@ -565,6 +622,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
func: &crate::Function,
stage: ShaderStage,
ep_name: &str,
frag_ep: Option<&FragmentEntryPoint<'_>>,
) -> Result<EntryPointInterface, Error> {
Ok(EntryPointInterface {
input: if !func.arguments.is_empty() && stage == ShaderStage::Fragment {
Expand All @@ -574,7 +632,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
},
output: match func.result {
Some(ref fr) if fr.binding.is_none() && stage == ShaderStage::Vertex => {
Some(self.write_ep_output_struct(module, fr, stage, ep_name)?)
Some(self.write_ep_output_struct(module, fr, stage, ep_name, frag_ep)?)
}
_ => None,
},
Expand Down
2 changes: 2 additions & 0 deletions naga/tests/in/unconsumed_vertex_outputs_frag.param.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
(
)
13 changes: 13 additions & 0 deletions naga/tests/in/unconsumed_vertex_outputs_frag.wgsl
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Out of order to test sorting.
struct FragmentIn {
@location(1) value: f32,
@location(3) value2: f32,
@builtin(position) position: vec4<f32>,
// @location(0) unused_value: f32,
// @location(2) unused_value2: vec4<f32>,
}

@fragment
fn fs_main(v_out: FragmentIn) -> @location(0) vec4<f32> {
return vec4<f32>(v_out.value, v_out.value, v_out.value2, v_out.value2);
}
2 changes: 2 additions & 0 deletions naga/tests/in/unconsumed_vertex_outputs_vert.param.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
(
)
13 changes: 13 additions & 0 deletions naga/tests/in/unconsumed_vertex_outputs_vert.wgsl
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Out of order to test sorting.
struct VertexOut {
@builtin(position) position: vec4<f32>,
@location(1) value: f32,
@location(2) unused_value2: vec4<f32>,
@location(0) unused_value: f32,
@location(3) value2: f32,
}

@vertex
fn vs_main() -> VertexOut {
return VertexOut(vec4(1.0), 1.0, vec4(2.0), 1.0, 0.5);
}
17 changes: 17 additions & 0 deletions naga/tests/out/hlsl/unconsumed_vertex_outputs_frag.hlsl
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
struct FragmentIn {
float value : LOC1;
float value2_ : LOC3;
float4 position : SV_Position;
};

struct FragmentInput_fs_main {
float value : LOC1;
float value2_ : LOC3;
float4 position : SV_Position;
};

float4 fs_main(FragmentInput_fs_main fragmentinput_fs_main) : SV_Target0
{
FragmentIn v_out = { fragmentinput_fs_main.value, fragmentinput_fs_main.value2_, fragmentinput_fs_main.position };
return float4(v_out.value, v_out.value, v_out.value2_, v_out.value2_);
}
12 changes: 12 additions & 0 deletions naga/tests/out/hlsl/unconsumed_vertex_outputs_frag.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
(
vertex:[
],
fragment:[
(
entry_point:"fs_main",
target_profile:"ps_5_1",
),
],
compute:[
],
)
30 changes: 30 additions & 0 deletions naga/tests/out/hlsl/unconsumed_vertex_outputs_vert.hlsl
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
struct VertexOut {
float4 position : SV_Position;
float value : LOC1;
float4 unused_value2_ : LOC2;
float unused_value : LOC0;
float value2_ : LOC3;
};

struct VertexOutput_vs_main {
float value : LOC1;
float value2_ : LOC3;
float4 position : SV_Position;
};

VertexOut ConstructVertexOut(float4 arg0, float arg1, float4 arg2, float arg3, float arg4) {
VertexOut ret = (VertexOut)0;
ret.position = arg0;
ret.value = arg1;
ret.unused_value2_ = arg2;
ret.unused_value = arg3;
ret.value2_ = arg4;
return ret;
}

VertexOutput_vs_main vs_main()
{
const VertexOut vertexout = ConstructVertexOut((1.0).xxxx, 1.0, (2.0).xxxx, 1.0, 0.5);
const VertexOutput_vs_main vertexout_1 = { vertexout.value, vertexout.value2_, vertexout.position };
return vertexout_1;
}
12 changes: 12 additions & 0 deletions naga/tests/out/hlsl/unconsumed_vertex_outputs_vert.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
(
vertex:[
(
entry_point:"vs_main",
target_profile:"vs_5_1",
),
],
fragment:[
],
compute:[
],
)

0 comments on commit 1ed945e

Please sign in to comment.