From 4b88dbcf8e14aefafe858c931acb36f3ca2c622e Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Thu, 16 Jun 2022 15:27:38 +0000 Subject: [PATCH] Fixup continue support in while loops. The generators were not setting `emit_continuing_` when emitting while loops. This caused a crash when a `continue` was encountered. This CL adds the `emit_continuing_` setup to the while emission. It also guards the `emit_continuing_` usage with making sure the function is setup. Bug: tint:1490 Change-Id: Ia89c49e567acda71a1f851a582103723cff71d49 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/93960 Commit-Queue: Dan Sinclair Auto-Submit: Dan Sinclair Kokoro: Kokoro Reviewed-by: Ben Clayton --- src/tint/writer/glsl/generator_impl.cc | 5 +- .../writer/glsl/generator_impl_loop_test.cc | 19 +++++++ src/tint/writer/hlsl/generator_impl.cc | 5 +- .../writer/hlsl/generator_impl_loop_test.cc | 19 +++++++ src/tint/writer/msl/generator_impl.cc | 5 +- .../writer/msl/generator_impl_loop_test.cc | 19 +++++++ .../writer/wgsl/generator_impl_loop_test.cc | 19 +++++++ test/tint/loops/while_with_continue.wgsl | 8 +++ .../while_with_continue.wgsl.expected.glsl | 15 ++++++ .../while_with_continue.wgsl.expected.hlsl | 13 +++++ .../while_with_continue.wgsl.expected.msl | 12 +++++ .../while_with_continue.wgsl.expected.spvasm | 51 +++++++++++++++++++ .../while_with_continue.wgsl.expected.wgsl | 8 +++ 13 files changed, 195 insertions(+), 3 deletions(-) create mode 100644 test/tint/loops/while_with_continue.wgsl create mode 100644 test/tint/loops/while_with_continue.wgsl.expected.glsl create mode 100644 test/tint/loops/while_with_continue.wgsl.expected.hlsl create mode 100644 test/tint/loops/while_with_continue.wgsl.expected.msl create mode 100644 test/tint/loops/while_with_continue.wgsl.expected.spvasm create mode 100644 test/tint/loops/while_with_continue.wgsl.expected.wgsl diff --git a/src/tint/writer/glsl/generator_impl.cc b/src/tint/writer/glsl/generator_impl.cc index 03ec75c14c..441b0d41f2 100644 --- a/src/tint/writer/glsl/generator_impl.cc +++ b/src/tint/writer/glsl/generator_impl.cc @@ -1753,7 +1753,7 @@ bool GeneratorImpl::EmitCase(const ast::CaseStatement* stmt) { } bool GeneratorImpl::EmitContinue(const ast::ContinueStatement*) { - if (!emit_continuing_()) { + if (!emit_continuing_ || !emit_continuing_()) { return false; } line() << "continue;"; @@ -2534,6 +2534,9 @@ bool GeneratorImpl::EmitWhile(const ast::WhileStatement* stmt) { } } + auto emit_continuing = [&]() { return true; }; + TINT_SCOPED_ASSIGNMENT(emit_continuing_, emit_continuing); + // If the whilehas a multi-statement conditional, then we cannot emit this // as a regular while in GLSL. Instead we need to generate a `while(true)` loop. bool emit_as_loop = cond_pre.lines.size() > 0; diff --git a/src/tint/writer/glsl/generator_impl_loop_test.cc b/src/tint/writer/glsl/generator_impl_loop_test.cc index 5f4d9554de..ec9219a9a4 100644 --- a/src/tint/writer/glsl/generator_impl_loop_test.cc +++ b/src/tint/writer/glsl/generator_impl_loop_test.cc @@ -400,6 +400,25 @@ TEST_F(GlslGeneratorImplTest_Loop, Emit_While) { )"); } +TEST_F(GlslGeneratorImplTest_Loop, Emit_While_WithContinue) { + // while(true) { + // continue; + // } + + auto* f = While(Expr(true), Block(Continue())); + WrapInFunction(f); + + GeneratorImpl& gen = Build(); + + gen.increment_indent(); + + ASSERT_TRUE(gen.EmitStatement(f)) << gen.error(); + EXPECT_EQ(gen.result(), R"( while(true) { + continue; + } +)"); +} + TEST_F(GlslGeneratorImplTest_Loop, Emit_WhileWithMultiStmtCond) { // while(true && false) { // return; diff --git a/src/tint/writer/hlsl/generator_impl.cc b/src/tint/writer/hlsl/generator_impl.cc index 39adcf83ed..b76b9e6cf6 100644 --- a/src/tint/writer/hlsl/generator_impl.cc +++ b/src/tint/writer/hlsl/generator_impl.cc @@ -2634,7 +2634,7 @@ bool GeneratorImpl::EmitCase(const ast::SwitchStatement* s, size_t case_idx) { } bool GeneratorImpl::EmitContinue(const ast::ContinueStatement*) { - if (!emit_continuing_()) { + if (!emit_continuing_ || !emit_continuing_()) { return false; } line() << "continue;"; @@ -3492,6 +3492,9 @@ bool GeneratorImpl::EmitWhile(const ast::WhileStatement* stmt) { } } + auto emit_continuing = [&]() { return true; }; + TINT_SCOPED_ASSIGNMENT(emit_continuing_, emit_continuing); + // If the while has a multi-statement conditional, then we cannot emit this // as a regular while in HLSL. Instead we need to generate a `while(true)` loop. bool emit_as_loop = cond_pre.lines.size() > 0; diff --git a/src/tint/writer/hlsl/generator_impl_loop_test.cc b/src/tint/writer/hlsl/generator_impl_loop_test.cc index 38ecb9bf3e..29d182264d 100644 --- a/src/tint/writer/hlsl/generator_impl_loop_test.cc +++ b/src/tint/writer/hlsl/generator_impl_loop_test.cc @@ -392,6 +392,25 @@ TEST_F(HlslGeneratorImplTest_Loop, Emit_While) { )"); } +TEST_F(HlslGeneratorImplTest_Loop, Emit_While_WithContinue) { + // while(true) { + // continue; + // } + + auto* f = While(Expr(true), Block(Continue())); + WrapInFunction(f); + + GeneratorImpl& gen = Build(); + + gen.increment_indent(); + + ASSERT_TRUE(gen.EmitStatement(f)) << gen.error(); + EXPECT_EQ(gen.result(), R"( [loop] while(true) { + continue; + } +)"); +} + TEST_F(HlslGeneratorImplTest_Loop, Emit_WhileWithMultiStmtCond) { // while(true && false) { // return; diff --git a/src/tint/writer/msl/generator_impl.cc b/src/tint/writer/msl/generator_impl.cc index 4d4c78de18..e5ba3a8e1a 100644 --- a/src/tint/writer/msl/generator_impl.cc +++ b/src/tint/writer/msl/generator_impl.cc @@ -1521,7 +1521,7 @@ bool GeneratorImpl::EmitCase(const ast::CaseStatement* stmt) { } bool GeneratorImpl::EmitContinue(const ast::ContinueStatement*) { - if (!emit_continuing_()) { + if (!emit_continuing_ || !emit_continuing_()) { return false; } @@ -2136,6 +2136,9 @@ bool GeneratorImpl::EmitWhile(const ast::WhileStatement* stmt) { } } + auto emit_continuing = [&]() { return true; }; + TINT_SCOPED_ASSIGNMENT(emit_continuing_, emit_continuing); + // If the while has a multi-statement conditional, then we cannot emit this // as a regular while in MSL. Instead we need to generate a `while(true)` loop. bool emit_as_loop = cond_pre.lines.size() > 0; diff --git a/src/tint/writer/msl/generator_impl_loop_test.cc b/src/tint/writer/msl/generator_impl_loop_test.cc index 73d88c0e1c..85f1debe03 100644 --- a/src/tint/writer/msl/generator_impl_loop_test.cc +++ b/src/tint/writer/msl/generator_impl_loop_test.cc @@ -363,6 +363,25 @@ TEST_F(MslGeneratorImplTest, Emit_While) { )"); } +TEST_F(MslGeneratorImplTest, Emit_While_WithContinue) { + // while(true) { + // continue; + // } + + auto* f = While(Expr(true), Block(Continue())); + WrapInFunction(f); + + GeneratorImpl& gen = Build(); + + gen.increment_indent(); + + ASSERT_TRUE(gen.EmitStatement(f)) << gen.error(); + EXPECT_EQ(gen.result(), R"( while(true) { + continue; + } +)"); +} + TEST_F(MslGeneratorImplTest, Emit_WhileWithMultiCond) { // while(true && false) { // return; diff --git a/src/tint/writer/wgsl/generator_impl_loop_test.cc b/src/tint/writer/wgsl/generator_impl_loop_test.cc index 99fcdb0799..3dbae60855 100644 --- a/src/tint/writer/wgsl/generator_impl_loop_test.cc +++ b/src/tint/writer/wgsl/generator_impl_loop_test.cc @@ -217,6 +217,25 @@ TEST_F(WgslGeneratorImplTest, Emit_While) { )"); } +TEST_F(WgslGeneratorImplTest, Emit_While_WithContinue) { + // while(true) { + // continue; + // } + + auto* f = While(Expr(true), Block(Continue())); + WrapInFunction(f); + + GeneratorImpl& gen = Build(); + + gen.increment_indent(); + + ASSERT_TRUE(gen.EmitStatement(f)) << gen.error(); + EXPECT_EQ(gen.result(), R"( while(true) { + continue; + } +)"); +} + TEST_F(WgslGeneratorImplTest, Emit_WhileMultiCond) { // while(true && false) { // return; diff --git a/test/tint/loops/while_with_continue.wgsl b/test/tint/loops/while_with_continue.wgsl new file mode 100644 index 0000000000..8193a0223d --- /dev/null +++ b/test/tint/loops/while_with_continue.wgsl @@ -0,0 +1,8 @@ +fn f() -> i32 { + var i : i32; + while (i < 4) { + i = i + 1; + continue; + } + return i; +} diff --git a/test/tint/loops/while_with_continue.wgsl.expected.glsl b/test/tint/loops/while_with_continue.wgsl.expected.glsl new file mode 100644 index 0000000000..bc66512a2b --- /dev/null +++ b/test/tint/loops/while_with_continue.wgsl.expected.glsl @@ -0,0 +1,15 @@ +#version 310 es + +layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; +void unused_entry_point() { + return; +} +int f() { + int i = 0; + while((i < 4)) { + i = (i + 1); + continue; + } + return i; +} + diff --git a/test/tint/loops/while_with_continue.wgsl.expected.hlsl b/test/tint/loops/while_with_continue.wgsl.expected.hlsl new file mode 100644 index 0000000000..c0d22da75a --- /dev/null +++ b/test/tint/loops/while_with_continue.wgsl.expected.hlsl @@ -0,0 +1,13 @@ +[numthreads(1, 1, 1)] +void unused_entry_point() { + return; +} + +int f() { + int i = 0; + [loop] while((i < 4)) { + i = (i + 1); + continue; + } + return i; +} diff --git a/test/tint/loops/while_with_continue.wgsl.expected.msl b/test/tint/loops/while_with_continue.wgsl.expected.msl new file mode 100644 index 0000000000..7fee4b083d --- /dev/null +++ b/test/tint/loops/while_with_continue.wgsl.expected.msl @@ -0,0 +1,12 @@ +#include + +using namespace metal; +int f() { + int i = 0; + while((i < 4)) { + i = as_type((as_type(i) + as_type(1))); + continue; + } + return i; +} + diff --git a/test/tint/loops/while_with_continue.wgsl.expected.spvasm b/test/tint/loops/while_with_continue.wgsl.expected.spvasm new file mode 100644 index 0000000000..cc53917841 --- /dev/null +++ b/test/tint/loops/while_with_continue.wgsl.expected.spvasm @@ -0,0 +1,51 @@ +; SPIR-V +; Version: 1.3 +; Generator: Google Tint Compiler; 0 +; Bound: 27 +; Schema: 0 + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint GLCompute %unused_entry_point "unused_entry_point" + OpExecutionMode %unused_entry_point LocalSize 1 1 1 + OpName %unused_entry_point "unused_entry_point" + OpName %f "f" + OpName %i "i" + %void = OpTypeVoid + %1 = OpTypeFunction %void + %int = OpTypeInt 32 1 + %5 = OpTypeFunction %int +%_ptr_Function_int = OpTypePointer Function %int + %11 = OpConstantNull %int + %int_4 = OpConstant %int 4 + %bool = OpTypeBool + %int_1 = OpConstant %int 1 +%unused_entry_point = OpFunction %void None %1 + %4 = OpLabel + OpReturn + OpFunctionEnd + %f = OpFunction %int None %5 + %8 = OpLabel + %i = OpVariable %_ptr_Function_int Function %11 + OpBranch %12 + %12 = OpLabel + OpLoopMerge %13 %14 None + OpBranch %15 + %15 = OpLabel + %17 = OpLoad %int %i + %19 = OpSLessThan %bool %17 %int_4 + %16 = OpLogicalNot %bool %19 + OpSelectionMerge %21 None + OpBranchConditional %16 %22 %21 + %22 = OpLabel + OpBranch %13 + %21 = OpLabel + %23 = OpLoad %int %i + %25 = OpIAdd %int %23 %int_1 + OpStore %i %25 + OpBranch %14 + %14 = OpLabel + OpBranch %12 + %13 = OpLabel + %26 = OpLoad %int %i + OpReturnValue %26 + OpFunctionEnd diff --git a/test/tint/loops/while_with_continue.wgsl.expected.wgsl b/test/tint/loops/while_with_continue.wgsl.expected.wgsl new file mode 100644 index 0000000000..bbbd5fdd18 --- /dev/null +++ b/test/tint/loops/while_with_continue.wgsl.expected.wgsl @@ -0,0 +1,8 @@ +fn f() -> i32 { + var i : i32; + while((i < 4)) { + i = (i + 1); + continue; + } + return i; +}