From e55823dc574695cc5d93715026580d5f54f1efe5 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Tue, 15 Nov 2022 14:21:45 +0100 Subject: [PATCH 1/9] [wgsl-in] use parse_block in more places --- src/front/wgsl/mod.rs | 41 ++++++++--------------------------------- 1 file changed, 8 insertions(+), 33 deletions(-) diff --git a/src/front/wgsl/mod.rs b/src/front/wgsl/mod.rs index 2873e6c73c..6117d0066a 100644 --- a/src/front/wgsl/mod.rs +++ b/src/front/wgsl/mod.rs @@ -3619,26 +3619,9 @@ impl Parser { return Ok(()); } (Token::Paren('{'), _) => { - self.push_rule_span(Rule::Block, lexer); - // Push a new lexical scope for the block statement - context.symbol_table.push_scope(); - - let _ = lexer.next(); - let mut statements = crate::Block::new(); - while !lexer.skip(Token::Paren('}')) { - self.parse_statement( - lexer, - context.reborrow(), - &mut statements, - is_uniform_control_flow, - )?; - } - // Pop the block statement lexical scope - context.symbol_table.pop_scope(); - - self.pop_rule_span(lexer); - let span = NagaSpan::from(self.pop_rule_span(lexer)); - block.push(crate::Statement::Block(statements), span); + let body = self.parse_block(lexer, context, is_uniform_control_flow)?; + let span = self.pop_rule_span(lexer); + block.push(crate::Statement::Block(body), NagaSpan::from(span)); return Ok(()); } (Token::Word(word), _) => { @@ -3993,7 +3976,6 @@ impl Parser { lexer, context.as_expression(&mut body, &mut emitter), )?; - lexer.expect(Token::Paren('{'))?; body.extend(emitter.finish(context.expressions)); Ok(condition) })?; @@ -4007,14 +3989,8 @@ impl Parser { }, NagaSpan::from(span), ); - // Push a lexical scope for the while loop body - context.symbol_table.push_scope(); - while !lexer.skip(Token::Paren('}')) { - self.parse_statement(lexer, context.reborrow(), &mut body, false)?; - } - // Pop the while loop body lexical scope - context.symbol_table.pop_scope(); + body.extend_block(self.parse_block(lexer, context.reborrow(), false)?); Some(crate::Statement::Loop { body, @@ -4093,11 +4069,9 @@ impl Parser { } lexer.expect(Token::Paren(')'))?; } - lexer.expect(Token::Paren('{'))?; - while !lexer.skip(Token::Paren('}')) { - self.parse_statement(lexer, context.reborrow(), &mut body, false)?; - } + body.extend_block(self.parse_block(lexer, context.reborrow(), false)?); + // Pop the for loop lexical scope context.symbol_table.pop_scope(); @@ -4306,6 +4280,7 @@ impl Parser { }) } + /// compound_statement fn parse_block<'a>( &mut self, lexer: &mut Lexer<'a>, @@ -4326,7 +4301,7 @@ impl Parser { is_uniform_control_flow, )?; } - //Pop the block lexical scope + // Pop the block lexical scope context.symbol_table.pop_scope(); self.pop_rule_span(lexer); From 5e99640a26b27e5d1f80f074d2a2b8fd1fb81b36 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Tue, 15 Nov 2022 14:29:11 +0100 Subject: [PATCH 2/9] [wgsl] remove fallthrough statement also add support for default to be used with other case selectors --- src/back/wgsl/writer.rs | 48 ++++++++---- src/front/wgsl/mod.rs | 74 +++++++------------ src/front/wgsl/tests.rs | 23 +++++- tests/in/control-flow.wgsl | 9 +-- tests/out/glsl/control-flow.main.Compute.glsl | 10 ++- tests/out/hlsl/control-flow.hlsl | 25 +++---- tests/out/msl/control-flow.msl | 9 ++- tests/out/spv/control-flow.spvasm | 46 ++++++------ tests/out/wgsl/control-flow.wgsl | 10 +-- tests/wgsl-errors.rs | 22 ------ 10 files changed, 138 insertions(+), 138 deletions(-) diff --git a/src/back/wgsl/writer.rs b/src/back/wgsl/writer.rs index 817fa78b0a..34dfba76a1 100644 --- a/src/back/wgsl/writer.rs +++ b/src/back/wgsl/writer.rs @@ -880,25 +880,47 @@ impl Writer { }; let l2 = level.next(); - if !cases.is_empty() { - for case in cases { - match case.value { - crate::SwitchValue::Integer(value) => { - writeln!(self.out, "{}case {}{}: {{", l2, value, type_postfix)?; + let mut new_case = true; + for case in cases { + if case.fall_through && !case.body.is_empty() { + // TODO: we could do the same workaround as we did for the HLSL backend + return Err(Error::Unimplemented( + "fall-through switch case block".into(), + )); + } + + match case.value { + crate::SwitchValue::Integer(value) => { + if new_case { + write!(self.out, "{}case ", l2)?; } - crate::SwitchValue::Default => { - writeln!(self.out, "{}default: {{", l2)?; + write!(self.out, "{}{}", value, type_postfix)?; + } + crate::SwitchValue::Default => { + if new_case { + if case.fall_through { + write!(self.out, "{}case ", l2)?; + } else { + write!(self.out, "{}", l2)?; + } } + write!(self.out, "default")?; } + } - for sta in case.body.iter() { - self.write_stmt(module, sta, func_ctx, l2.next())?; - } + new_case = !case.fall_through; - if case.fall_through { - writeln!(self.out, "{}fallthrough;", l2.next())?; - } + if case.fall_through { + write!(self.out, ", ")?; + } else { + writeln!(self.out, ": {{")?; + } + + for sta in case.body.iter() { + self.write_stmt(module, sta, func_ctx, l2.next())?; + } + if !case.fall_through { writeln!(self.out, "{}}}", l2)?; } } diff --git a/src/front/wgsl/mod.rs b/src/front/wgsl/mod.rs index 6117d0066a..4e2733f69e 100644 --- a/src/front/wgsl/mod.rs +++ b/src/front/wgsl/mod.rs @@ -1513,13 +1513,20 @@ impl Parser { lexer.span_from(initial) } - fn parse_switch_value<'a>(lexer: &mut Lexer<'a>, uint: bool) -> Result> { - let token_span = lexer.next(); - match token_span.0 { - Token::Number(Ok(Number::U32(num))) if uint => Ok(num as i32), - Token::Number(Ok(Number::I32(num))) if !uint => Ok(num), - Token::Number(Err(e)) => Err(Error::BadNumber(token_span.1, e)), - _ => Err(Error::Unexpected(token_span.1, ExpectedToken::Integer)), + fn parse_switch_value<'a>( + lexer: &mut Lexer<'a>, + uint: bool, + ) -> Result> { + match lexer.next() { + (Token::Word("default"), _) => Ok(crate::SwitchValue::Default), + (Token::Number(Ok(Number::U32(num))), _) if uint => { + Ok(crate::SwitchValue::Integer(num as i32)) + } + (Token::Number(Ok(Number::I32(num))), _) if !uint => { + Ok(crate::SwitchValue::Integer(num)) + } + (Token::Number(Err(e)), span) => Err(Error::BadNumber(span, e)), + (_, span) => Err(Error::Unexpected(span, ExpectedToken::Integer)), } } @@ -3576,34 +3583,6 @@ impl Parser { Ok(()) } - fn parse_switch_case_body<'a, 'out>( - &mut self, - lexer: &mut Lexer<'a>, - mut context: StatementContext<'a, '_, 'out>, - ) -> Result<(bool, crate::Block), Error<'a>> { - let mut body = crate::Block::new(); - // Push a new lexical scope for the switch case body - context.symbol_table.push_scope(); - - lexer.expect(Token::Paren('{'))?; - let fall_through = loop { - // default statements - if lexer.skip(Token::Word("fallthrough")) { - lexer.expect(Token::Separator(';'))?; - lexer.expect(Token::Paren('}'))?; - break true; - } - if lexer.skip(Token::Paren('}')) { - break false; - } - self.parse_statement(lexer, context.reborrow(), &mut body, false)?; - }; - // Pop the switch case body lexical scope - context.symbol_table.pop_scope(); - - Ok((fall_through, body)) - } - fn parse_statement<'a, 'out>( &mut self, lexer: &mut Lexer<'a>, @@ -3928,37 +3907,34 @@ impl Parser { break value; } cases.push(crate::SwitchCase { - value: crate::SwitchValue::Integer(value), + value, body: crate::Block::new(), fall_through: true, }); }; - let (fall_through, body) = - self.parse_switch_case_body(lexer, context.reborrow())?; - + let body = + self.parse_block(lexer, context.reborrow(), false)?; cases.push(crate::SwitchCase { - value: crate::SwitchValue::Integer(value), + value, body, - fall_through, + fall_through: false, }); } (Token::Word("default"), _) => { lexer.skip(Token::Separator(':')); - let (fall_through, body) = - self.parse_switch_case_body(lexer, context.reborrow())?; + + let body = + self.parse_block(lexer, context.reborrow(), false)?; cases.push(crate::SwitchCase { value: crate::SwitchValue::Default, body, - fall_through, + fall_through: false, }); } (Token::Paren('}'), _) => break, - other => { - return Err(Error::Unexpected( - other.1, - ExpectedToken::SwitchItem, - )) + (_, span) => { + return Err(Error::Unexpected(span, ExpectedToken::SwitchItem)) } } } diff --git a/src/front/wgsl/tests.rs b/src/front/wgsl/tests.rs index 33fc541acb..6aaa505d4d 100644 --- a/src/front/wgsl/tests.rs +++ b/src/front/wgsl/tests.rs @@ -249,8 +249,7 @@ fn parse_switch() { var pos: f32; switch (3) { case 0, 1: { pos = 0.0; } - case 2: { pos = 1.0; fallthrough; } - case 3: {} + case 2: { pos = 1.0; } default: { pos = 3.0; } } } @@ -267,8 +266,7 @@ fn parse_switch_optional_colon_in_case() { var pos: f32; switch (3) { case 0, 1 { pos = 0.0; } - case 2 { pos = 1.0; fallthrough; } - case 3 {} + case 2 { pos = 1.0; } default { pos = 3.0; } } } @@ -277,6 +275,23 @@ fn parse_switch_optional_colon_in_case() { .unwrap(); } +#[test] +fn parse_switch_default_in_case() { + parse_str( + " + fn main() { + var pos: f32; + switch (3) { + case 0, 1: { pos = 0.0; } + case 2: {} + case default, 3: { pos = 3.0; } + } + } + ", + ) + .unwrap(); +} + #[test] fn parse_parentheses_switch() { parse_str( diff --git a/tests/in/control-flow.wgsl b/tests/in/control-flow.wgsl index 787742d71d..5a0ef1cbbf 100644 --- a/tests/in/control-flow.wgsl +++ b/tests/in/control-flow.wgsl @@ -22,15 +22,13 @@ fn main(@builtin(global_invocation_id) global_id: vec3) { case 2: { pos = 1; } - case 3: { + case 3, 4: { pos = 2; - fallthrough; } - case 4: { + case 5: { pos = 3; - fallthrough; } - default: { + case default, 6: { pos = 4; } } @@ -54,7 +52,6 @@ fn main(@builtin(global_invocation_id) global_id: vec3) { } case 3: { pos = 2; - fallthrough; } case 4: {} default: { diff --git a/tests/out/glsl/control-flow.main.Compute.glsl b/tests/out/glsl/control-flow.main.Compute.glsl index 5446e36098..a66c83a309 100644 --- a/tests/out/glsl/control-flow.main.Compute.glsl +++ b/tests/out/glsl/control-flow.main.Compute.glsl @@ -56,12 +56,16 @@ void main() { pos = 1; break; case 3: - pos = 2; /* fallthrough */ case 4: + pos = 2; + break; + case 5: pos = 3; - /* fallthrough */ + break; default: + /* fallthrough */ + case 6: pos = 4; break; } @@ -81,7 +85,7 @@ void main() { return; case 3: pos = 2; - /* fallthrough */ + return; case 4: return; default: diff --git a/tests/out/hlsl/control-flow.hlsl b/tests/out/hlsl/control-flow.hlsl index 4b879ef20c..354c552a15 100644 --- a/tests/out/hlsl/control-flow.hlsl +++ b/tests/out/hlsl/control-flow.hlsl @@ -61,26 +61,29 @@ void main(uint3 global_id : SV_DispatchThreadID) } case 3: { { - pos = 2; - } - { - pos = 3; } { - pos = 4; + pos = 2; } break; } case 4: { + pos = 2; + break; + } + case 5: { + pos = 3; + break; + } + default: { { - pos = 3; } { pos = 4; } break; } - default: { + case 6: { pos = 4; break; } @@ -104,12 +107,8 @@ void main(uint3 global_id : SV_DispatchThreadID) return; } case 3: { - { - pos = 2; - } - { - return; - } + pos = 2; + return; } case 4: { return; diff --git a/tests/out/msl/control-flow.msl b/tests/out/msl/control-flow.msl index 4a0b805663..d00b7ec18b 100644 --- a/tests/out/msl/control-flow.msl +++ b/tests/out/msl/control-flow.msl @@ -69,12 +69,18 @@ kernel void main_( break; } case 3: { - pos = 2; } case 4: { + pos = 2; + break; + } + case 5: { pos = 3; + break; } default: { + } + case 6: { pos = 4; break; } @@ -99,6 +105,7 @@ kernel void main_( } case 3: { pos = 2; + return; } case 4: { return; diff --git a/tests/out/spv/control-flow.spvasm b/tests/out/spv/control-flow.spvasm index dfd1b145a2..fa00d2dba9 100644 --- a/tests/out/spv/control-flow.spvasm +++ b/tests/out/spv/control-flow.spvasm @@ -1,7 +1,7 @@ ; SPIR-V ; Version: 1.1 ; Generator: rspirv -; Bound: 69 +; Bound: 71 OpCapability Shader %1 = OpExtInstImport "GLSL.std.450" OpMemoryModel Logical GLSL450 @@ -92,7 +92,7 @@ OpBranch %50 %50 = OpLabel %52 = OpLoad %4 %37 OpSelectionMerge %53 None -OpSwitch %52 %54 1 %55 2 %56 3 %57 4 %58 +OpSwitch %52 %54 1 %55 2 %56 3 %57 4 %58 5 %59 %55 = OpLabel OpStore %37 %5 OpBranch %53 @@ -100,39 +100,43 @@ OpBranch %53 OpStore %37 %3 OpBranch %53 %57 = OpLabel -OpStore %37 %6 OpBranch %58 %58 = OpLabel +OpStore %37 %6 +OpBranch %53 +%59 = OpLabel OpStore %37 %7 -OpBranch %54 +OpBranch %53 %54 = OpLabel +OpBranch %60 +%60 = OpLabel OpStore %37 %8 OpBranch %53 %53 = OpLabel -OpSelectionMerge %59 None -OpSwitch %9 %60 0 %61 +OpSelectionMerge %61 None +OpSwitch %9 %62 0 %63 +%63 = OpLabel +OpBranch %61 +%62 = OpLabel +OpBranch %61 %61 = OpLabel -OpBranch %59 -%60 = OpLabel -OpBranch %59 -%59 = OpLabel -%62 = OpLoad %4 %37 -OpSelectionMerge %63 None -OpSwitch %62 %64 1 %65 2 %66 3 %67 4 %68 -%65 = OpLabel +%64 = OpLoad %4 %37 +OpSelectionMerge %65 None +OpSwitch %64 %66 1 %67 2 %68 3 %69 4 %70 +%67 = OpLabel OpStore %37 %5 -OpBranch %63 -%66 = OpLabel +OpBranch %65 +%68 = OpLabel OpStore %37 %3 OpReturn -%67 = OpLabel +%69 = OpLabel OpStore %37 %6 -OpBranch %68 -%68 = OpLabel OpReturn -%64 = OpLabel +%70 = OpLabel +OpReturn +%66 = OpLabel OpStore %37 %7 OpReturn -%63 = OpLabel +%65 = OpLabel OpReturn OpFunctionEnd \ No newline at end of file diff --git a/tests/out/wgsl/control-flow.wgsl b/tests/out/wgsl/control-flow.wgsl index ce3911f2b1..2872406d76 100644 --- a/tests/out/wgsl/control-flow.wgsl +++ b/tests/out/wgsl/control-flow.wgsl @@ -50,15 +50,13 @@ fn main(@builtin(global_invocation_id) global_id: vec3) { case 2: { pos = 1; } - case 3: { + case 3, 4: { pos = 2; - fallthrough; } - case 4: { + case 5: { pos = 3; - fallthrough; } - default: { + case default, 6: { pos = 4; } } @@ -80,7 +78,7 @@ fn main(@builtin(global_invocation_id) global_id: vec3) { } case 3: { pos = 2; - fallthrough; + return; } case 4: { return; diff --git a/tests/wgsl-errors.rs b/tests/wgsl-errors.rs index dd3e3dda6c..594d92354d 100644 --- a/tests/wgsl-errors.rs +++ b/tests/wgsl-errors.rs @@ -1370,28 +1370,6 @@ fn select() { } } -#[test] -fn last_case_falltrough() { - check_validation! { - " - fn test_falltrough() { - switch(0) { - default: {} - case 0: { - fallthrough; - } - } - } - ": - Err( - naga::valid::ValidationError::Function { - source: naga::valid::FunctionError::LastCaseFallTrough, - .. - }, - ) - } -} - #[test] fn missing_default_case() { check_validation! { From d1948e3a03fe701e4526dbd68d2ca8121c3fa20b Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Tue, 15 Nov 2022 14:31:21 +0100 Subject: [PATCH 3/9] [msl-out] omit extra switch case blocks where possible --- src/back/msl/writer.rs | 17 ++++++++++++++--- tests/out/msl/control-flow.msl | 6 ++---- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index 7617ca9a6d..7cf6ebed62 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -2546,19 +2546,30 @@ impl Writer { for case in cases.iter() { match case.value { crate::SwitchValue::Integer(value) => { - writeln!(self.out, "{}case {}{}: {{", lcase, value, type_postfix)?; + write!(self.out, "{}case {}{}:", lcase, value, type_postfix)?; } crate::SwitchValue::Default => { - writeln!(self.out, "{}default: {{", lcase)?; + write!(self.out, "{}default:", lcase)?; } } + + let write_block_braces = !(case.fall_through && case.body.is_empty()); + if write_block_braces { + writeln!(self.out, " {{")?; + } else { + writeln!(self.out)?; + } + self.put_block(lcase.next(), &case.body, context)?; if !case.fall_through && case.body.last().map_or(true, |s| !s.is_terminator()) { writeln!(self.out, "{}break;", lcase.next())?; } - writeln!(self.out, "{}}}", lcase)?; + + if write_block_braces { + writeln!(self.out, "{}}}", lcase)?; + } } writeln!(self.out, "{}}}", level)?; } diff --git a/tests/out/msl/control-flow.msl b/tests/out/msl/control-flow.msl index d00b7ec18b..be396e23a8 100644 --- a/tests/out/msl/control-flow.msl +++ b/tests/out/msl/control-flow.msl @@ -68,8 +68,7 @@ kernel void main_( pos = 1; break; } - case 3: { - } + case 3: case 4: { pos = 2; break; @@ -78,8 +77,7 @@ kernel void main_( pos = 3; break; } - default: { - } + default: case 6: { pos = 4; break; From 69342d154856cea2ee1f91a206a304c26ec2e5f6 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Tue, 15 Nov 2022 14:32:00 +0100 Subject: [PATCH 4/9] [hlsl-out] omit extra switch case blocks where possible --- src/back/hlsl/writer.rs | 23 +++++++++++++++++------ tests/out/hlsl/control-flow.hlsl | 18 ++---------------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/src/back/hlsl/writer.rs b/src/back/hlsl/writer.rs index c0f2442309..0c6ce9bb66 100644 --- a/src/back/hlsl/writer.rs +++ b/src/back/hlsl/writer.rs @@ -1825,18 +1825,25 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { for (i, case) in cases.iter().enumerate() { match case.value { - crate::SwitchValue::Integer(value) => writeln!( + crate::SwitchValue::Integer(value) => write!( self.out, - "{}case {}{}: {{", + "{}case {}{}:", indent_level_1, value, type_postfix )?, crate::SwitchValue::Default => { - writeln!(self.out, "{}default: {{", indent_level_1)? + write!(self.out, "{}default:", indent_level_1)? } } + let write_block_braces = !(case.fall_through && case.body.is_empty()); + if write_block_braces { + writeln!(self.out, " {{")?; + } else { + writeln!(self.out)?; + } + // FXC doesn't support fallthrough so we duplicate the body of the following case blocks - if case.fall_through { + if case.fall_through && !case.body.is_empty() { let curr_len = i + 1; let end_case_idx = curr_len + cases @@ -1861,12 +1868,16 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { for sta in case.body.iter() { self.write_stmt(module, sta, func_ctx, indent_level_2)?; } - if case.body.last().map_or(true, |s| !s.is_terminator()) { + if !case.fall_through + && case.body.last().map_or(true, |s| !s.is_terminator()) + { writeln!(self.out, "{}break;", indent_level_2)?; } } - writeln!(self.out, "{}}}", indent_level_1)?; + if write_block_braces { + writeln!(self.out, "{}}}", indent_level_1)?; + } } writeln!(self.out, "{}}}", level)? diff --git a/tests/out/hlsl/control-flow.hlsl b/tests/out/hlsl/control-flow.hlsl index 354c552a15..7f7a4b6e5c 100644 --- a/tests/out/hlsl/control-flow.hlsl +++ b/tests/out/hlsl/control-flow.hlsl @@ -59,14 +59,7 @@ void main(uint3 global_id : SV_DispatchThreadID) pos = 1; break; } - case 3: { - { - } - { - pos = 2; - } - break; - } + case 3: case 4: { pos = 2; break; @@ -75,14 +68,7 @@ void main(uint3 global_id : SV_DispatchThreadID) pos = 3; break; } - default: { - { - } - { - pos = 4; - } - break; - } + default: case 6: { pos = 4; break; From 5c87a4e9b5c8d635dcf2aee5f8cb61c579daad00 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Tue, 15 Nov 2022 14:36:52 +0100 Subject: [PATCH 5/9] [spv-out] fix switch cases after default not being output --- src/back/spv/block.rs | 25 ++++--------------------- tests/out/spv/control-flow.spvasm | 2 +- 2 files changed, 5 insertions(+), 22 deletions(-) diff --git a/src/back/spv/block.rs b/src/back/spv/block.rs index 10fd5d72aa..3c32ca85c7 100644 --- a/src/back/spv/block.rs +++ b/src/back/spv/block.rs @@ -1682,26 +1682,20 @@ impl<'w> BlockContext<'w> { let default_id = self.gen_id(); - let mut reached_default = false; let mut raw_cases = Vec::with_capacity(cases.len()); let mut case_ids = Vec::with_capacity(cases.len()); for case in cases.iter() { match case.value { crate::SwitchValue::Integer(value) => { let label_id = self.gen_id(); - // No cases should be added after the default case is encountered - // since the default case catches all - if !reached_default { - raw_cases.push(super::instructions::Case { - value: value as Word, - label_id, - }); - } + raw_cases.push(super::instructions::Case { + value: value as Word, + label_id, + }); case_ids.push(label_id); } crate::SwitchValue::Default => { case_ids.push(default_id); - reached_default = true; } } } @@ -1732,17 +1726,6 @@ impl<'w> BlockContext<'w> { )?; } - // If no default was encountered write a empty block to satisfy the presence of - // a block the default label - if !reached_default { - self.write_block( - default_id, - &[], - BlockExit::Branch { target: merge_id }, - inner_context, - )?; - } - block = Block::new(merge_id); } crate::Statement::Loop { diff --git a/tests/out/spv/control-flow.spvasm b/tests/out/spv/control-flow.spvasm index fa00d2dba9..cc986fe775 100644 --- a/tests/out/spv/control-flow.spvasm +++ b/tests/out/spv/control-flow.spvasm @@ -92,7 +92,7 @@ OpBranch %50 %50 = OpLabel %52 = OpLoad %4 %37 OpSelectionMerge %53 None -OpSwitch %52 %54 1 %55 2 %56 3 %57 4 %58 5 %59 +OpSwitch %52 %54 1 %55 2 %56 3 %57 4 %58 5 %59 6 %60 %55 = OpLabel OpStore %37 %5 OpBranch %53 From 18804044daa8c9c51bf7ed2083031a3ab2316058 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Tue, 15 Nov 2022 14:39:39 +0100 Subject: [PATCH 6/9] [spv-out] omit extra switch case blocks where possible --- src/back/spv/block.rs | 25 ++++++++++--- tests/out/spv/control-flow.spvasm | 62 +++++++++++++++---------------- 2 files changed, 49 insertions(+), 38 deletions(-) diff --git a/src/back/spv/block.rs b/src/back/spv/block.rs index 3c32ca85c7..728af63c26 100644 --- a/src/back/spv/block.rs +++ b/src/back/spv/block.rs @@ -1680,26 +1680,36 @@ impl<'w> BlockContext<'w> { spirv::SelectionControl::NONE, )); - let default_id = self.gen_id(); + let mut default_id = None; + // id of previous empty fall-through case + let mut last_id = None; let mut raw_cases = Vec::with_capacity(cases.len()); let mut case_ids = Vec::with_capacity(cases.len()); for case in cases.iter() { + let label_id = last_id.take().unwrap_or_else(|| self.gen_id()); + + if case.fall_through && case.body.is_empty() { + last_id = Some(label_id); + } + + case_ids.push(label_id); + match case.value { crate::SwitchValue::Integer(value) => { - let label_id = self.gen_id(); raw_cases.push(super::instructions::Case { value: value as Word, label_id, }); - case_ids.push(label_id); } crate::SwitchValue::Default => { - case_ids.push(default_id); + default_id = Some(label_id); } } } + let default_id = default_id.unwrap(); + self.function.consume( block, Instruction::switch(selector_id, default_id, &raw_cases), @@ -1710,7 +1720,12 @@ impl<'w> BlockContext<'w> { ..loop_context }; - for (i, (case, label_id)) in cases.iter().zip(case_ids.iter()).enumerate() { + for (i, (case, label_id)) in cases + .iter() + .zip(case_ids.iter()) + .filter(|&(case, _)| !(case.fall_through && case.body.is_empty())) + .enumerate() + { let case_finish_id = if case.fall_through { case_ids[i + 1] } else { diff --git a/tests/out/spv/control-flow.spvasm b/tests/out/spv/control-flow.spvasm index cc986fe775..e1885e7159 100644 --- a/tests/out/spv/control-flow.spvasm +++ b/tests/out/spv/control-flow.spvasm @@ -1,7 +1,7 @@ ; SPIR-V ; Version: 1.1 ; Generator: rspirv -; Bound: 71 +; Bound: 69 OpCapability Shader %1 = OpExtInstImport "GLSL.std.450" OpMemoryModel Logical GLSL450 @@ -45,11 +45,11 @@ OpFunctionEnd OpBranch %22 %22 = OpLabel OpSelectionMerge %23 None -OpSwitch %5 %24 0 %25 -%25 = OpLabel -OpBranch %23 +OpSwitch %5 %25 0 %24 %24 = OpLabel OpBranch %23 +%25 = OpLabel +OpBranch %23 %23 = OpLabel OpReturn OpFunctionEnd @@ -64,10 +64,10 @@ OpLoopMerge %31 %33 None OpBranch %32 %32 = OpLabel OpSelectionMerge %34 None -OpSwitch %27 %35 1 %36 -%36 = OpLabel -OpBranch %33 +OpSwitch %27 %36 1 %35 %35 = OpLabel +OpBranch %33 +%36 = OpLabel OpBranch %34 %34 = OpLabel OpBranch %33 @@ -92,51 +92,47 @@ OpBranch %50 %50 = OpLabel %52 = OpLoad %4 %37 OpSelectionMerge %53 None -OpSwitch %52 %54 1 %55 2 %56 3 %57 4 %58 5 %59 6 %60 -%55 = OpLabel +OpSwitch %52 %58 1 %54 2 %55 3 %56 4 %56 5 %57 6 %58 +%54 = OpLabel OpStore %37 %5 OpBranch %53 -%56 = OpLabel +%55 = OpLabel OpStore %37 %3 OpBranch %53 -%57 = OpLabel -OpBranch %58 -%58 = OpLabel +%56 = OpLabel OpStore %37 %6 OpBranch %53 -%59 = OpLabel +%57 = OpLabel OpStore %37 %7 OpBranch %53 -%54 = OpLabel -OpBranch %60 -%60 = OpLabel +%58 = OpLabel OpStore %37 %8 OpBranch %53 %53 = OpLabel -OpSelectionMerge %61 None -OpSwitch %9 %62 0 %63 -%63 = OpLabel -OpBranch %61 -%62 = OpLabel -OpBranch %61 +OpSelectionMerge %59 None +OpSwitch %9 %61 0 %60 +%60 = OpLabel +OpBranch %59 %61 = OpLabel -%64 = OpLoad %4 %37 -OpSelectionMerge %65 None -OpSwitch %64 %66 1 %67 2 %68 3 %69 4 %70 -%67 = OpLabel +OpBranch %59 +%59 = OpLabel +%62 = OpLoad %4 %37 +OpSelectionMerge %63 None +OpSwitch %62 %68 1 %64 2 %65 3 %66 4 %67 +%64 = OpLabel OpStore %37 %5 -OpBranch %65 -%68 = OpLabel +OpBranch %63 +%65 = OpLabel OpStore %37 %3 OpReturn -%69 = OpLabel +%66 = OpLabel OpStore %37 %6 OpReturn -%70 = OpLabel +%67 = OpLabel OpReturn -%66 = OpLabel +%68 = OpLabel OpStore %37 %7 OpReturn -%65 = OpLabel +%63 = OpLabel OpReturn OpFunctionEnd \ No newline at end of file From 833ce7d3392aeea99cca0dc2424c59c0b0636506 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Thu, 17 Nov 2022 18:10:19 +0100 Subject: [PATCH 7/9] [glsl-out] introduce a new block for switch cases --- src/back/glsl/mod.rs | 22 +++++--- tests/out/glsl/control-flow.main.Compute.glsl | 56 ++++++++++++------- 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/src/back/glsl/mod.rs b/src/back/glsl/mod.rs index 3698b79781..72a5d46f77 100644 --- a/src/back/glsl/mod.rs +++ b/src/back/glsl/mod.rs @@ -1807,23 +1807,29 @@ impl<'a, W: Write> Writer<'a, W> { for case in cases { match case.value { crate::SwitchValue::Integer(value) => { - writeln!(self.out, "{}case {}{}:", l2, value, type_postfix)? + write!(self.out, "{}case {}{}:", l2, value, type_postfix)? } - crate::SwitchValue::Default => writeln!(self.out, "{}default:", l2)?, + crate::SwitchValue::Default => write!(self.out, "{}default:", l2)?, + } + + let write_block_braces = !(case.fall_through && case.body.is_empty()); + if write_block_braces { + writeln!(self.out, " {{")?; + } else { + writeln!(self.out)?; } for sta in case.body.iter() { self.write_stmt(sta, ctx, l2.next())?; } - // Write fallthrough comment if the case is fallthrough, - // otherwise write a break, if the case is not already - // broken out of at the end of its body. - if case.fall_through { - writeln!(self.out, "{}/* fallthrough */", l2.next())?; - } else if case.body.last().map_or(true, |s| !s.is_terminator()) { + if !case.fall_through && case.body.last().map_or(true, |s| !s.is_terminator()) { writeln!(self.out, "{}break;", l2.next())?; } + + if write_block_braces { + writeln!(self.out, "{}}}", l2)?; + } } writeln!(self.out, "{}}}", level)? diff --git a/tests/out/glsl/control-flow.main.Compute.glsl b/tests/out/glsl/control-flow.main.Compute.glsl index a66c83a309..b877f9cb69 100644 --- a/tests/out/glsl/control-flow.main.Compute.glsl +++ b/tests/out/glsl/control-flow.main.Compute.glsl @@ -8,17 +8,20 @@ layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; void switch_default_break(int i) { switch(i) { - default: + default: { break; + } } } void switch_case_break() { switch(0) { - case 0: + case 0: { break; - default: + } + default: { break; + } } return; } @@ -26,10 +29,12 @@ void switch_case_break() { void loop_switch_continue(int x) { while(true) { switch(x) { - case 1: + case 1: { continue; - default: + } + default: { break; + } } } return; @@ -43,54 +48,65 @@ void main() { memoryBarrierShared(); barrier(); switch(1) { - default: + default: { pos = 1; break; + } } int _e4 = pos; switch(_e4) { - case 1: + case 1: { pos = 0; break; - case 2: + } + case 2: { pos = 1; break; + } case 3: - /* fallthrough */ - case 4: + case 4: { pos = 2; break; - case 5: + } + case 5: { pos = 3; break; + } default: - /* fallthrough */ - case 6: + case 6: { pos = 4; break; + } } switch(0u) { - case 0u: + case 0u: { break; - default: + } + default: { break; + } } int _e11 = pos; switch(_e11) { - case 1: + case 1: { pos = 0; break; - case 2: + } + case 2: { pos = 1; return; - case 3: + } + case 3: { pos = 2; return; - case 4: + } + case 4: { return; - default: + } + default: { pos = 3; return; + } } } From e9f817d5c8b4e69a90d8344c1293f93b80afcdb0 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Thu, 17 Nov 2022 18:34:54 +0100 Subject: [PATCH 8/9] [doc] explain how case clauses with multiple selectors are supported --- src/lib.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index e122d1224c..abd6cb858b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1466,6 +1466,22 @@ pub enum Statement { reject: Block, }, /// Conditionally executes one of multiple blocks, based on the value of the selector. + /// + /// Each case must have a distinct [`value`], exactly one of which must be + /// [`Default`]. The `Default` may appear at any position, and covers all + /// values not explicitly appearing in other cases. A `Default` appearing in + /// the midst of the list of cases does not shadow the cases that follow. + /// + /// Some backend languages don't support fallthrough (HLSL due to FXC, + /// WGSL), and may translate fallthrough cases in the IR by duplicating + /// code. However, all backend languages do support cases selected by + /// multiple values, like `case 1: case 2: case 3: { ... }`. This is + /// represented in the IR as a series of fallthrough cases with empty + /// bodies, except for the last. + /// + /// [`value`]: SwitchCase::value + /// [`body`]: SwitchCase::body + /// [`Default`]: SwitchValue::Default Switch { selector: Handle, //int cases: Vec, From cbf84b9d351ed793c884de58b4e4823a08d1bb69 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Fri, 9 Dec 2022 12:37:27 +0100 Subject: [PATCH 9/9] [doc] add more switch docs --- src/back/hlsl/writer.rs | 24 +++++++++++++++++++++++- src/back/spv/block.rs | 1 + 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/back/hlsl/writer.rs b/src/back/hlsl/writer.rs index 0c6ce9bb66..e1325e5abf 100644 --- a/src/back/hlsl/writer.rs +++ b/src/back/hlsl/writer.rs @@ -1835,6 +1835,12 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { } } + // The new block is not only stylistic, it plays a role here: + // We might end up having to write the same case body + // multiple times due to FXC not supporting fallthrough. + // Therefore, some `Expression`s written by `Statement::Emit` + // will end up having the same name (`_expr`). + // So we need to put each case in its own scope. let write_block_braces = !(case.fall_through && case.body.is_empty()); if write_block_braces { writeln!(self.out, " {{")?; @@ -1842,7 +1848,23 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { writeln!(self.out)?; } - // FXC doesn't support fallthrough so we duplicate the body of the following case blocks + // Although FXC does support a series of case clauses before + // a block[^yes], it does not support fallthrough from a + // non-empty case block to the next[^no]. If this case has a + // non-empty body with a fallthrough, emulate that by + // duplicating the bodies of all the cases it would fall + // into as extensions of this case's own body. This makes + // the HLSL output potentially quadratic in the size of the + // Naga IR. + // + // [^yes]: ```hlsl + // case 1: + // case 2: do_stuff() + // ``` + // [^no]: ```hlsl + // case 1: do_this(); + // case 2: do_that(); + // ``` if case.fall_through && !case.body.is_empty() { let curr_len = i + 1; let end_case_idx = curr_len diff --git a/src/back/spv/block.rs b/src/back/spv/block.rs index 728af63c26..9bf0b8c895 100644 --- a/src/back/spv/block.rs +++ b/src/back/spv/block.rs @@ -1687,6 +1687,7 @@ impl<'w> BlockContext<'w> { let mut raw_cases = Vec::with_capacity(cases.len()); let mut case_ids = Vec::with_capacity(cases.len()); for case in cases.iter() { + // take id of previous empty fall-through case or generate a new one let label_id = last_id.take().unwrap_or_else(|| self.gen_id()); if case.fall_through && case.body.is_empty() {