commit eab1f626295c64225e95862b35e2a359559bbdc7
author Antonio Maiorano <amaiorano@google.com> Wed Feb 01 15:46:34 2023 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Feb 01 15:46:34 2023 +0000
tree 04a6568d5a387a6de5fc4a0df7a5547c5cfb1fe3
parent 98bd83a8fca4b358885ea77a17faa2c4a8a7a768

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 <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>

src/tint/writer/hlsl/generator_impl.cc

diff --git a/src/tint/writer/hlsl/generator_impl.cc b/src/tint/writer/hlsl/generator_impl.cc
index bde9d70..d49bcc9 100644
--- a/src/tint/writer/hlsl/generator_impl.cc
+++ b/src/tint/writer/hlsl/generator_impl.cc
@@ -3859,10 +3859,9 @@
     // 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;

src/tint/writer/hlsl/generator_impl_switch_test.cc

diff --git a/src/tint/writer/hlsl/generator_impl_switch_test.cc b/src/tint/writer/hlsl/generator_impl_switch_test.cc
index 698083a..6426b7c 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_OnlyDefaultCase) {
+TEST_F(HlslGeneratorImplTest_Switch, Emit_Switch_OnlyDefaultCase_NoSideEffectsCondition) {
+    // var<private> cond : i32;
+    // var<private> 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 @@
     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<private> global : i32;
+    // fn bar() -> i32 {
+    //   global = 84;
+    //   return global;
+    // }
+    //
+    // var<private> 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);