From eab1f626295c64225e95862b35e2a359559bbdc7 Mon Sep 17 00:00:00 2001 From: Antonio Maiorano Date: Wed, 1 Feb 2023 15:46:34 +0000 Subject: [PATCH] tint/hlsl: for default-only switch, only emit condition if it has side-effects This fixes edge-cases, like the condition expression being a type-cast, which DXC apparently sees as a variable re-declaration. Example: fn foo(x : f32) { switch (i32(x)) { default { } } } was emitted as HLSL: void foo(float x) { int(x); do { } while (false); } The `int(x)` is seen as a re-declaration of `x` by DXC. We fix this by only emitted the condition expression if it has side-effects (which currently means it contains a call expression). Bug: tint:1820 Change-Id: I7e4320fa09ea2d634c9e324cb0b752b0ee7dcde9 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/118161 Kokoro: Kokoro Reviewed-by: James Price Reviewed-by: Ben Clayton Commit-Queue: Antonio Maiorano --- src/tint/writer/hlsl/generator_impl.cc | 7 +- .../writer/hlsl/generator_impl_switch_test.cc | 51 ++++++++++++++- test/tint/bug/tint/1820.wgsl | 22 +++++++ .../tint/bug/tint/1820.wgsl.expected.dxc.hlsl | 25 +++++++ .../tint/bug/tint/1820.wgsl.expected.fxc.hlsl | 25 +++++++ test/tint/bug/tint/1820.wgsl.expected.glsl | 31 +++++++++ test/tint/bug/tint/1820.wgsl.expected.msl | 28 ++++++++ test/tint/bug/tint/1820.wgsl.expected.spvasm | 65 +++++++++++++++++++ test/tint/bug/tint/1820.wgsl.expected.wgsl | 23 +++++++ .../only_default_case.wgsl.expected.dxc.hlsl | 1 - .../only_default_case.wgsl.expected.fxc.hlsl | 1 - 11 files changed, 271 insertions(+), 8 deletions(-) create mode 100644 test/tint/bug/tint/1820.wgsl create mode 100644 test/tint/bug/tint/1820.wgsl.expected.dxc.hlsl create mode 100644 test/tint/bug/tint/1820.wgsl.expected.fxc.hlsl create mode 100644 test/tint/bug/tint/1820.wgsl.expected.glsl create mode 100644 test/tint/bug/tint/1820.wgsl.expected.msl create mode 100644 test/tint/bug/tint/1820.wgsl.expected.spvasm create mode 100644 test/tint/bug/tint/1820.wgsl.expected.wgsl diff --git a/src/tint/writer/hlsl/generator_impl.cc b/src/tint/writer/hlsl/generator_impl.cc index bde9d703f3..d49bcc92e6 100644 --- a/src/tint/writer/hlsl/generator_impl.cc +++ b/src/tint/writer/hlsl/generator_impl.cc @@ -3859,10 +3859,9 @@ bool GeneratorImpl::EmitDefaultOnlySwitch(const ast::SwitchStatement* stmt) { // default case body. We work around this here by emitting the default case // without the switch. - // Emit the switch condition as-is in case it has side-effects (e.g. - // function call). Note that's it's fine not to assign the result of the - // expression. - { + // Emit the switch condition as-is if it has side-effects (e.g. + // function call). Note that we can ignore the result of the expression (if any). + if (auto* sem_cond = builder_.Sem().Get(stmt->condition); sem_cond->HasSideEffects()) { auto out = line(); if (!EmitExpression(out, stmt->condition)) { return false; diff --git a/src/tint/writer/hlsl/generator_impl_switch_test.cc b/src/tint/writer/hlsl/generator_impl_switch_test.cc index 698083a03e..6426b7c343 100644 --- a/src/tint/writer/hlsl/generator_impl_switch_test.cc +++ b/src/tint/writer/hlsl/generator_impl_switch_test.cc @@ -66,7 +66,16 @@ TEST_F(HlslGeneratorImplTest_Switch, Emit_Switch_MixedDefault) { )"); } -TEST_F(HlslGeneratorImplTest_Switch, Emit_Switch_OnlyDefaultCase) { +TEST_F(HlslGeneratorImplTest_Switch, Emit_Switch_OnlyDefaultCase_NoSideEffectsCondition) { + // var cond : i32; + // var a : i32; + // fn test() { + // switch(cond) { + // default: { + // a = 42; + // } + // } + // } GlobalVar("cond", ty.i32(), type::AddressSpace::kPrivate); GlobalVar("a", ty.i32(), type::AddressSpace::kPrivate); auto* s = Switch( // @@ -79,7 +88,45 @@ TEST_F(HlslGeneratorImplTest_Switch, Emit_Switch_OnlyDefaultCase) { gen.increment_indent(); ASSERT_TRUE(gen.EmitStatement(s)) << gen.error(); - EXPECT_EQ(gen.result(), R"( cond; + EXPECT_EQ(gen.result(), R"( do { + a = 42; + } while (false); +)"); +} + +TEST_F(HlslGeneratorImplTest_Switch, Emit_Switch_OnlyDefaultCase_SideEffectsCondition) { + // var global : i32; + // fn bar() -> i32 { + // global = 84; + // return global; + // } + // + // var a : i32; + // fn test() { + // switch(bar()) { + // default: { + // a = 42; + // } + // } + // } + GlobalVar("global", ty.i32(), type::AddressSpace::kPrivate); + Func("bar", {}, ty.i32(), + utils::Vector{ // + Assign("global", Expr(84_i)), // + Return("global")}); + + GlobalVar("a", ty.i32(), type::AddressSpace::kPrivate); + auto* s = Switch( // + Call("bar"), // + DefaultCase(Block(Assign(Expr("a"), Expr(42_i))))); + WrapInFunction(s); + + GeneratorImpl& gen = Build(); + + gen.increment_indent(); + + ASSERT_TRUE(gen.EmitStatement(s)) << gen.error(); + EXPECT_EQ(gen.result(), R"( bar(); do { a = 42; } while (false); diff --git a/test/tint/bug/tint/1820.wgsl b/test/tint/bug/tint/1820.wgsl new file mode 100644 index 0000000000..781a980e03 --- /dev/null +++ b/test/tint/bug/tint/1820.wgsl @@ -0,0 +1,22 @@ +fn foo(x : f32) { + switch (i32(x)) { + default { + } + } +} + +var global : i32; +fn baz(x : i32) -> i32 { + global = 42; + return x; +} + +fn bar(x : f32) { + switch (baz(i32(x))) { + default { + } + } +} + +fn main() { +} diff --git a/test/tint/bug/tint/1820.wgsl.expected.dxc.hlsl b/test/tint/bug/tint/1820.wgsl.expected.dxc.hlsl new file mode 100644 index 0000000000..5e7dc3966c --- /dev/null +++ b/test/tint/bug/tint/1820.wgsl.expected.dxc.hlsl @@ -0,0 +1,25 @@ +[numthreads(1, 1, 1)] +void unused_entry_point() { + return; +} + +void foo(float x) { + do { + } while (false); +} + +static int global = 0; + +int baz(int x) { + global = 42; + return x; +} + +void bar(float x) { + baz(int(x)); + do { + } while (false); +} + +void main() { +} diff --git a/test/tint/bug/tint/1820.wgsl.expected.fxc.hlsl b/test/tint/bug/tint/1820.wgsl.expected.fxc.hlsl new file mode 100644 index 0000000000..5e7dc3966c --- /dev/null +++ b/test/tint/bug/tint/1820.wgsl.expected.fxc.hlsl @@ -0,0 +1,25 @@ +[numthreads(1, 1, 1)] +void unused_entry_point() { + return; +} + +void foo(float x) { + do { + } while (false); +} + +static int global = 0; + +int baz(int x) { + global = 42; + return x; +} + +void bar(float x) { + baz(int(x)); + do { + } while (false); +} + +void main() { +} diff --git a/test/tint/bug/tint/1820.wgsl.expected.glsl b/test/tint/bug/tint/1820.wgsl.expected.glsl new file mode 100644 index 0000000000..9fa7dd083a --- /dev/null +++ b/test/tint/bug/tint/1820.wgsl.expected.glsl @@ -0,0 +1,31 @@ +#version 310 es + +layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; +void unused_entry_point() { + return; +} +void foo(float x) { + switch(int(x)) { + default: { + break; + } + } +} + +int global = 0; +int baz(int x) { + global = 42; + return x; +} + +void bar(float x) { + switch(baz(int(x))) { + default: { + break; + } + } +} + +void tint_symbol() { +} + diff --git a/test/tint/bug/tint/1820.wgsl.expected.msl b/test/tint/bug/tint/1820.wgsl.expected.msl new file mode 100644 index 0000000000..e811b1bcf5 --- /dev/null +++ b/test/tint/bug/tint/1820.wgsl.expected.msl @@ -0,0 +1,28 @@ +#include + +using namespace metal; +void foo(float x) { + switch(int(x)) { + default: { + break; + } + } +} + +int baz(int x) { + thread int tint_symbol_1 = 0; + tint_symbol_1 = 42; + return x; +} + +void bar(float x) { + switch(baz(int(x))) { + default: { + break; + } + } +} + +void tint_symbol() { +} + diff --git a/test/tint/bug/tint/1820.wgsl.expected.spvasm b/test/tint/bug/tint/1820.wgsl.expected.spvasm new file mode 100644 index 0000000000..1a4bc10008 --- /dev/null +++ b/test/tint/bug/tint/1820.wgsl.expected.spvasm @@ -0,0 +1,65 @@ +; SPIR-V +; Version: 1.3 +; Generator: Google Tint Compiler; 0 +; Bound: 31 +; Schema: 0 + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint GLCompute %unused_entry_point "unused_entry_point" + OpExecutionMode %unused_entry_point LocalSize 1 1 1 + OpName %global "global" + OpName %unused_entry_point "unused_entry_point" + OpName %foo "foo" + OpName %x "x" + OpName %baz "baz" + OpName %x_0 "x" + OpName %bar "bar" + OpName %x_1 "x" + OpName %main "main" + %int = OpTypeInt 32 1 +%_ptr_Private_int = OpTypePointer Private %int + %4 = OpConstantNull %int + %global = OpVariable %_ptr_Private_int Private %4 + %void = OpTypeVoid + %5 = OpTypeFunction %void + %float = OpTypeFloat 32 + %9 = OpTypeFunction %void %float + %17 = OpTypeFunction %int %int + %int_42 = OpConstant %int 42 +%unused_entry_point = OpFunction %void None %5 + %8 = OpLabel + OpReturn + OpFunctionEnd + %foo = OpFunction %void None %9 + %x = OpFunctionParameter %float + %13 = OpLabel + %15 = OpConvertFToS %int %x + OpSelectionMerge %14 None + OpSwitch %15 %16 + %16 = OpLabel + OpBranch %14 + %14 = OpLabel + OpReturn + OpFunctionEnd + %baz = OpFunction %int None %17 + %x_0 = OpFunctionParameter %int + %20 = OpLabel + OpStore %global %int_42 + OpReturnValue %x_0 + OpFunctionEnd + %bar = OpFunction %void None %9 + %x_1 = OpFunctionParameter %float + %24 = OpLabel + %27 = OpConvertFToS %int %x_1 + %26 = OpFunctionCall %int %baz %27 + OpSelectionMerge %25 None + OpSwitch %26 %28 + %28 = OpLabel + OpBranch %25 + %25 = OpLabel + OpReturn + OpFunctionEnd + %main = OpFunction %void None %5 + %30 = OpLabel + OpReturn + OpFunctionEnd diff --git a/test/tint/bug/tint/1820.wgsl.expected.wgsl b/test/tint/bug/tint/1820.wgsl.expected.wgsl new file mode 100644 index 0000000000..5fa9cd9487 --- /dev/null +++ b/test/tint/bug/tint/1820.wgsl.expected.wgsl @@ -0,0 +1,23 @@ +fn foo(x : f32) { + switch(i32(x)) { + default: { + } + } +} + +var global : i32; + +fn baz(x : i32) -> i32 { + global = 42; + return x; +} + +fn bar(x : f32) { + switch(baz(i32(x))) { + default: { + } + } +} + +fn main() { +} diff --git a/test/tint/statements/switch/only_default_case.wgsl.expected.dxc.hlsl b/test/tint/statements/switch/only_default_case.wgsl.expected.dxc.hlsl index 128b8a2393..ded0d6647d 100644 --- a/test/tint/statements/switch/only_default_case.wgsl.expected.dxc.hlsl +++ b/test/tint/statements/switch/only_default_case.wgsl.expected.dxc.hlsl @@ -2,7 +2,6 @@ void f() { int i = 0; int result = 0; - i; do { result = 44; break; diff --git a/test/tint/statements/switch/only_default_case.wgsl.expected.fxc.hlsl b/test/tint/statements/switch/only_default_case.wgsl.expected.fxc.hlsl index 128b8a2393..ded0d6647d 100644 --- a/test/tint/statements/switch/only_default_case.wgsl.expected.fxc.hlsl +++ b/test/tint/statements/switch/only_default_case.wgsl.expected.fxc.hlsl @@ -2,7 +2,6 @@ void f() { int i = 0; int result = 0; - i; do { result = 44; break;