Skip to content

Commit

Permalink
[wgsl-out] Correct handling of named pointer expressions.
Browse files Browse the repository at this point in the history
Treat expressions in `Function::named_expressions` like WGSL `let` declarations,
assuming that the Load Rule was applied to the rhs of the declaration, meaning
that their values are always `Indirection::Ordinary`.

Split `write_expr_plain_form` out from `write_expr_with_indirection`, to clean
up the parenthesis generation: no more `opened_paren` variable, just function
calls. This makes the early return for named expressions neater.

Fixes #1382.
  • Loading branch information
jimblandy committed Sep 17, 2021
1 parent 152444a commit ae97a3f
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 21 deletions.
2 changes: 1 addition & 1 deletion src/back/hlsl/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
}
_ => {
return Err(Error::Unimplemented(format!(
"write_value_type {:?}",
"back::hlsl::Writer::write_value_type {:?}",
inner
)))
}
Expand Down
64 changes: 44 additions & 20 deletions src/back/wgsl/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ impl<W: Write> Writer<W> {
}
_ => {
return Err(Error::Unimplemented(format!(
"write_value_type {:?}",
"back::wgsl::Writer::write_value_type {:?}",
inner
)));
}
Expand Down Expand Up @@ -991,11 +991,20 @@ impl<W: Write> Writer<W> {
/// argument's value directly, so any pointer it produces is merely the value
/// passed by the caller.
fn plain_form_indirection(
&self,
expr: Handle<crate::Expression>,
module: &Module,
func_ctx: &back::FunctionCtx<'_>,
) -> Indirection {
use crate::Expression as Ex;

// Named expressions are `let` expressions, which apply the Load Rule,
// so if their type is a Naga pointer, then that must be a WGSL pointer
// as well.
if self.named_expressions.contains_key(&expr) {
return Indirection::Ordinary;
}

match func_ctx.expressions[expr] {
Ex::LocalVariable(_) => Indirection::Reference,
Ex::GlobalVariable(handle) => {
Expand Down Expand Up @@ -1073,27 +1082,46 @@ impl<W: Write> Writer<W> {
func_ctx: &back::FunctionCtx<'_>,
requested: Indirection,
) -> BackendResult {
use crate::Expression;

if let Some(name) = self.named_expressions.get(&expr) {
write!(self.out, "{}", name)?;
return Ok(());
}

// If the plain form of the expression is not what we need, emit the
// operator necessary to correct that.
let plain = Self::plain_form_indirection(expr, module, func_ctx);
let opened_paren = match (requested, plain) {
let plain = self.plain_form_indirection(expr, module, func_ctx);
match (requested, plain) {
(Indirection::Ordinary, Indirection::Reference) => {
write!(self.out, "(&")?;
true
self.write_expr_plain_form(module, expr, func_ctx, plain)?;
write!(self.out, ")")?;
}
(Indirection::Reference, Indirection::Ordinary) => {
write!(self.out, "(*")?;
true
self.write_expr_plain_form(module, expr, func_ctx, plain)?;
write!(self.out, ")")?;
}
(_, _) => false,
};
(_, _) => self.write_expr_plain_form(module, expr, func_ctx, plain)?,
}

Ok(())
}

/// Write the 'plain form' of `expr`.
///
/// An expression's 'plain form' is the most general rendition of that
/// expression into WGSL, lacking `&` or `*` operators. The plain forms of
/// `LocalVariable(x)` and `GlobalVariable(g)` are simply `x` and `g`. Such
/// Naga expressions represent both WGSL pointers and references; it's the
/// caller's responsibility to distinguish those cases appropriately.
fn write_expr_plain_form(
&mut self,
module: &Module,
expr: Handle<crate::Expression>,
func_ctx: &back::FunctionCtx<'_>,
indirection: Indirection,
) -> BackendResult {
use crate::Expression;

if let Some(name) = self.named_expressions.get(&expr) {
write!(self.out, "{}", name)?;
return Ok(());
}

let expression = &func_ctx.expressions[expr];

Expand Down Expand Up @@ -1166,7 +1194,7 @@ impl<W: Write> Writer<W> {
}
// TODO: copy-paste from glsl-out
Expression::Access { base, index } => {
self.write_expr_with_indirection(module, base, func_ctx, plain)?;
self.write_expr_with_indirection(module, base, func_ctx, indirection)?;
write!(self.out, "[")?;
self.write_expr(module, index, func_ctx)?;
write!(self.out, "]")?
Expand All @@ -1176,7 +1204,7 @@ impl<W: Write> Writer<W> {
let base_ty_res = &func_ctx.info[base].ty;
let mut resolved = base_ty_res.inner_with(&module.types);

self.write_expr_with_indirection(module, base, func_ctx, plain)?;
self.write_expr_with_indirection(module, base, func_ctx, indirection)?;

let base_ty_handle = match *resolved {
TypeInner::Pointer { base, class: _ } => {
Expand Down Expand Up @@ -1598,10 +1626,6 @@ impl<W: Write> Writer<W> {
Expression::CallResult(_) | Expression::AtomicResult { .. } => {}
}

if opened_paren {
write!(self.out, ")")?
};

Ok(())
}

Expand Down
7 changes: 7 additions & 0 deletions tests/in/pointers.param.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
(
spv: (
version: (1, 2),
debug: true,
adjust_coordinate_space: false,
),
)
5 changes: 5 additions & 0 deletions tests/in/pointers.wgsl
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
fn f() {
var v: vec2<i32>;
let px = &v.x;
*px = 10;
}
29 changes: 29 additions & 0 deletions tests/out/spv/pointers.spvasm
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
; SPIR-V
; Version: 1.2
; Generator: rspirv
; Bound: 16
OpCapability Shader
OpCapability Linkage
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpSource GLSL 450
OpName %6 "v"
OpName %9 "f"
%2 = OpTypeVoid
%4 = OpTypeInt 32 1
%3 = OpConstant %4 10
%5 = OpTypeVector %4 2
%7 = OpTypePointer Function %5
%10 = OpTypeFunction %2
%12 = OpTypePointer Function %4
%14 = OpTypeInt 32 0
%13 = OpConstant %14 0
%9 = OpFunction %2 None %10
%8 = OpLabel
%6 = OpVariable %7 Function
OpBranch %11
%11 = OpLabel
%15 = OpAccessChain %12 %6 %13
OpStore %15 %3
OpReturn
OpFunctionEnd
8 changes: 8 additions & 0 deletions tests/out/wgsl/pointers.wgsl
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
fn f() {
var v: vec2<i32>;

let px: ptr<function, i32> = (&v.x);
(*px) = 10;
return;
}

1 change: 1 addition & 0 deletions tests/snapshots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,7 @@ fn convert_wgsl() {
"access",
Targets::SPIRV | Targets::METAL | Targets::GLSL | Targets::HLSL | Targets::WGSL,
),
("pointers", Targets::SPIRV | Targets::WGSL),
(
"control-flow",
Targets::SPIRV | Targets::METAL | Targets::GLSL | Targets::HLSL | Targets::WGSL,
Expand Down

0 comments on commit ae97a3f

Please sign in to comment.