From a1bda83c424f296691032f97eaca888bbe137b9d Mon Sep 17 00:00:00 2001
From: David Neto <dneto@google.com>
Date: Sat, 18 Feb 2023 07:05:58 +0000
Subject: [PATCH] spirv-reader: don't hoist opaque objects

Fixed: tint:1839
Change-Id: I9738c390ab78e8dceb25572ea7cac6bdc0661304
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/120460
Commit-Queue: David Neto <dneto@google.com>
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
---
 src/tint/reader/spirv/function.cc             |  5 ++
 .../reader/spirv/parser_impl_handle_test.cc   | 77 +++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/src/tint/reader/spirv/function.cc b/src/tint/reader/spirv/function.cc
index 82c7310e6a..d63a8f11d3 100644
--- a/src/tint/reader/spirv/function.cc
+++ b/src/tint/reader/spirv/function.cc
@@ -5038,6 +5038,11 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() {
             // private variables.
             continue;
         }
+        if (def_info->skip == SkipReason::kOpaqueObject) {
+            // Intermediate values are never emitted for opaque objects. So they
+            // need neither hoisted let or var declarations.
+            continue;
+        }
         auto& local_def = def_info->local.value();
 
         if (local_def.num_uses == 0) {
diff --git a/src/tint/reader/spirv/parser_impl_handle_test.cc b/src/tint/reader/spirv/parser_impl_handle_test.cc
index 1270d31af7..fbf73e4290 100644
--- a/src/tint/reader/spirv/parser_impl_handle_test.cc
+++ b/src/tint/reader/spirv/parser_impl_handle_test.cc
@@ -3828,6 +3828,83 @@ return;
     ASSERT_EQ(expect, got);
 }
 
+TEST_F(SpvParserHandleTest, SamplerLoadedInEnclosingConstruct_DontGenerateVar) {
+    // crbug.com/tint/1839
+    // When a sampler is loaded in an enclosing structued construct, don't
+    // generate a variable for it. The ordinary tracing logic will find the
+    // originating variable anyway.
+    const auto assembly = Preamble() + R"(
+     OpEntryPoint Fragment %100 "main"
+     OpExecutionMode %100 OriginUpperLeft
+
+     OpName %var_im "var_im"
+     OpName %var_s "var_s"
+     OpDecorate %var_im DescriptorSet 0
+     OpDecorate %var_im Binding 0
+     OpDecorate %var_s DescriptorSet 0
+     OpDecorate %var_s Binding 1
+  %float = OpTypeFloat 32
+  %v4float = OpTypeVector %float 4
+  %v2float = OpTypeVector %float 2
+  %v2_0 = OpConstantNull %v2float
+     %im = OpTypeImage %float 2D 0 0 0 1 Unknown
+     %si = OpTypeSampledImage %im
+      %s = OpTypeSampler
+ %ptr_im = OpTypePointer UniformConstant %im
+  %ptr_s = OpTypePointer UniformConstant %s
+ %var_im = OpVariable %ptr_im UniformConstant
+  %var_s = OpVariable %ptr_s UniformConstant
+   %void = OpTypeVoid
+ %voidfn = OpTypeFunction %void
+ %ptr_v4 = OpTypePointer Function %v4float
+   %bool = OpTypeBool
+   %true = OpConstantTrue %bool
+    %int = OpTypeInt 32 1
+  %int_0 = OpConstant %int 0
+
+    %100 = OpFunction %void None %voidfn
+  %entry = OpLabel
+      %1 = OpLoad %im %var_im
+      %2 = OpLoad %s %var_s
+           OpSelectionMerge %90 None
+           OpSwitch %int_0 %20
+
+     %20 = OpLabel
+           OpSelectionMerge %80 None
+           OpBranchConditional %true %30 %80
+
+     %30 = OpLabel
+    %si0 = OpSampledImage %si %1 %2
+     %t0 = OpImageSampleImplicitLod %v4float %si0 %v2_0
+           OpBranch %80
+
+     %80 = OpLabel
+           OpBranch %90
+
+     %90 = OpLabel
+           OpReturn
+           OpFunctionEnd
+  )";
+    auto p = parser(test::Assemble(assembly));
+    std::cout << assembly;
+    EXPECT_TRUE(p->BuildAndParseInternalModule()) << assembly;
+    auto fe = p->function_emitter(100);
+    EXPECT_TRUE(fe.EmitBody()) << p->error();
+    EXPECT_TRUE(p->error().empty()) << p->error();
+    auto ast_body = fe.ast_body();
+    const auto got = test::ToString(p->program(), ast_body);
+    auto* expect = R"(switch(0i) {
+  default: {
+    if (true) {
+      let x_24 : vec4<f32> = textureSample(var_im, var_s, vec2<f32>());
+    }
+  }
+}
+return;
+)";
+    ASSERT_EQ(expect, got);
+}
+
 TEST_F(SpvParserHandleTest, ImageCoordinateCanBeHoistedConstant) {
     // Demonstrates fix for crbug.com/tint/1646
     // The problem is the coordinate for an image operation