[spirv-reader] Don't hoist pointers that are already in scope.

Bug: tint:3, tint:213
Change-Id: I79410c670b8690c1f1ea86b7b9427f272a4ebbbb
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/27720
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: dan sinclair <dsinclair@chromium.org>

src/reader/spirv/function.cc

@@ -3247,12 +3247,21 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() {
       // Update the usage span for IDs used by this instruction.
       // But skip uses in OpPhi because they are handled differently.
       if (inst.opcode() != SpvOpPhi) {
-        inst.ForEachInId([this, block_pos](const uint32_t* id_ptr) {
+        inst.ForEachInId([this, block_pos, block_info](const uint32_t* id_ptr) {
           auto* def_info = GetDefInfo(*id_ptr);
           if (def_info) {
             def_info->num_uses++;
             def_info->last_use_pos =
                 std::max(def_info->last_use_pos, block_pos);
+
+            // Determine whether this ID is defined in a different construct
+            // from this use.
+            const auto defining_block = block_order_[def_info->block_pos];
+            const auto* def_in_construct =
+                GetBlockInfo(defining_block)->construct;
+            if (def_in_construct != block_info->construct) {
+              def_info->used_in_another_construct = true;
+            }
           }
         });
       }
@@ -3317,8 +3326,6 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() {
     const auto first_pos = def_info->block_pos;
     const auto last_use_pos = def_info->last_use_pos;
 
-    const auto* const construct_with_last_use =
-        GetBlockInfo(block_order_[last_use_pos])->construct;
     const auto* def_in_construct =
         GetBlockInfo(block_order_[first_pos])->construct;
     // A definition in the first block of an kIfSelection or kSwitchSelection
@@ -3331,7 +3338,22 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() {
       }
     }
 
-    if (def_in_construct != construct_with_last_use) {
+    bool should_hoist = false;
+    if (!def_in_construct->ContainsPos(last_use_pos)) {
+      // To satisfy scoping, we have to hoist the definition out to an enclosing
+      // construct.
+      should_hoist = true;
+    } else {
+      // Avoid moving combinatorial values across constructs.  This is a
+      // simple heuristic to avoid changing the cost of an operation
+      // by moving it into or out of a loop, for example.
+      if ((def_info->storage_class == ast::StorageClass::kNone) &&
+          def_info->used_in_another_construct) {
+        should_hoist = true;
+      }
+    }
+
+    if (should_hoist) {
       const auto* enclosing_construct =
           GetEnclosingScope(first_pos, last_use_pos);
       if (enclosing_construct == def_in_construct) {

src/reader/spirv/function.h

@@ -223,6 +223,10 @@ struct DefInfo {
   /// at all.  The "last" ordering is determined by the function block order.
   uint32_t last_use_pos = 0;
 
+  /// Is this value used in a construct other than the one in which it was
+  /// defined?
+  bool used_in_another_construct = false;
+
   /// True if this ID requires a WGSL 'const' definition, due to context. It
   /// might get one anyway (so this is *not* an if-and-only-if condition).
   bool requires_named_const_def = false;
@@ -248,7 +252,7 @@ struct DefInfo {
   std::string phi_var;
 
   /// The storage class to use for this value, if it is of pointer type.
-  /// This is required to carry a stroage class override from a storage
+  /// This is required to carry a storage class override from a storage
   /// buffer expressed in the old style (with Uniform storage class)
   /// that needs to be remapped to StorageBuffer storage class.
   /// This is kNone for non-pointers.

src/reader/spirv/function_var_test.cc

@@ -728,7 +728,7 @@ TEST_F(SpvParserTest,
        EmitStatement_CombinatorialValue_Immediate_UsedOnceDifferentConstruct) {
   // Translation should not sink expensive operations into or out of control
   // flow. As a simple heuristic, don't move *any* combinatorial operation
-  // across any constrol flow.
+  // across any control flow.
   auto assembly = Preamble() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -1182,6 +1182,74 @@ Return{}
 )")) << ToString(fe.ast_body());
 }
 
+TEST_F(SpvParserTest,
+       EmitStatement_CombinatorialNonPointer_Hoisting_DefAndUseFirstBlockIf) {
+  // In this test, both the defintion and the use are in the first block
+  // of an IfSelection.  No hoisting occurs because hoisting is triggered
+  // on whether the defining construct contains the last use, rather than
+  // whether the two constructs are the same.
+  //
+  // This example has two SSA IDs which are tempting to hoist but should not:
+  //   %1 is defined and used in the first block of an IfSelection.
+  //       Do not hoist it.
+  auto assembly = Preamble() + R"(
+     %cond = OpConstantTrue %bool
+
+     %100 = OpFunction %void None %voidfn
+
+     ; in IfSelection construct, nested in Function construct
+     %10 = OpLabel
+     %1 = OpCopyObject %uint %uint_1
+     %2 = OpCopyObject %uint %1
+     OpSelectionMerge %99 None
+     OpBranchConditional %cond %20 %99
+
+     %20 = OpLabel  ; in IfSelection construct
+     OpBranch %99
+
+     %99 = OpLabel
+     OpReturn
+
+     OpFunctionEnd
+  )";
+  auto* p = parser(test::Assemble(assembly));
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
+  FunctionEmitter fe(p, *spirv_function(100));
+  EXPECT_TRUE(fe.EmitBody()) << p->error();
+
+  // We don't hoist x_1 into its own mutable variable. It is emitted as
+  // a const definition.
+  EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(VariableDeclStatement{
+  Variable{
+    x_1
+    none
+    __u32
+    {
+      ScalarConstructor{1}
+    }
+  }
+}
+VariableDeclStatement{
+  Variable{
+    x_2
+    none
+    __u32
+    {
+      Identifier{x_1}
+    }
+  }
+}
+If{
+  (
+    ScalarConstructor{true}
+  )
+  {
+  }
+}
+Return{}
+)")) << ToString(fe.ast_body());
+}
+
 TEST_F(SpvParserTest, EmitStatement_Phi_SingleBlockLoopIndex) {
   auto assembly = Preamble() + R"(
      %pty = OpTypePointer Private %uint