spirv-reader: Don't hoist builtin vars.

Fixed: tint:1577
Change-Id: I1decc99e4a5a293b49c24aa15d1ac6dd0287462e
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/93680
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Auto-Submit: David Neto <dneto@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>

src/tint/reader/spirv/function.cc

@@ -725,9 +725,13 @@ BlockInfo::BlockInfo(const spvtools::opt::BasicBlock& bb) : basic_block(&bb), id
 BlockInfo::~BlockInfo() = default;
 
 DefInfo::DefInfo(const spvtools::opt::Instruction& def_inst,
+                 bool the_locally_defined,
                  uint32_t the_block_pos,
                  size_t the_index)
-    : inst(def_inst), block_pos(the_block_pos), index(the_index) {}
+    : inst(def_inst),
+      locally_defined(the_locally_defined),
+      block_pos(the_block_pos),
+      index(the_index) {}
 
 DefInfo::~DefInfo() = default;
 
@@ -4479,9 +4483,10 @@ bool FunctionEmitter::RegisterSpecialBuiltInVariables() {
         const auto id = special_var.first;
         const auto builtin = special_var.second;
         const auto* var = def_use_mgr_->GetDef(id);
-        def_info_[id] = std::make_unique<DefInfo>(*var, 0, index);
+        def_info_[id] = std::make_unique<DefInfo>(*var, false, 0, index);
         ++index;
         auto& def = def_info_[id];
+        // Builtins are always defined outside the function.
         switch (builtin) {
             case SpvBuiltInPointSize:
                 def->skip = SkipReason::kPointSizeBuiltinPointer;
@@ -4526,7 +4531,7 @@ bool FunctionEmitter::RegisterLocallyDefinedValues() {
             if ((result_id == 0) || inst.opcode() == SpvOpLabel) {
                 continue;
             }
-            def_info_[result_id] = std::make_unique<DefInfo>(inst, block_pos, index);
+            def_info_[result_id] = std::make_unique<DefInfo>(inst, true, block_pos, index);
             ++index;
             auto& info = def_info_[result_id];
 
@@ -4722,6 +4727,13 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() {
             // There is no need to adjust the location of the declaration.
             continue;
         }
+        if (!def_info->locally_defined) {
+            // Never hoist a variable declared at module scope.
+            // This occurs for builtin variables, which are mapped to module-scope
+            // private variables.
+            continue;
+        }
+
         // The first use must be the at the SSA definition, because block order
         // respects dominance.
         const auto first_pos = def_info->block_pos;

src/tint/reader/spirv/function.h

@@ -241,14 +241,22 @@ enum class SkipReason {
 struct DefInfo {
     /// Constructor.
     /// @param def_inst the SPIR-V instruction defining the ID
+    /// @param locally_defined the SPIR-V instruction defining the ID
     /// @param block_pos the position of the basic block where the ID is defined.
     /// @param index an ordering index for this local definition
-    DefInfo(const spvtools::opt::Instruction& def_inst, uint32_t block_pos, size_t index);
+    DefInfo(const spvtools::opt::Instruction& def_inst,
+            bool locally_defined,
+            uint32_t block_pos,
+            size_t index);
     /// Destructor.
     ~DefInfo();
 
     /// The SPIR-V instruction that defines the ID.
     const spvtools::opt::Instruction& inst;
+
+    /// True if the definition of this ID is inside the function.
+    const bool locally_defined = true;
+
     /// The position of the first block in which this ID is visible, in function
     /// block order.  For IDs defined outside of the function, it is 0.
     /// For IDs defined in the function, it is the position of the block
@@ -320,8 +328,11 @@ struct DefInfo {
 /// @returns the ostream so calls can be chained
 inline std::ostream& operator<<(std::ostream& o, const DefInfo& di) {
     o << "DefInfo{"
-      << " inst.result_id: " << di.inst.result_id() << " block_pos: " << di.block_pos
-      << " num_uses: " << di.num_uses << " last_use_pos: " << di.last_use_pos
+      << " inst.result_id: " << di.inst.result_id()
+      << " locally_defined: " << (di.locally_defined ? "true" : "false")
+      << " block_pos: " << di.block_pos << " num_uses: " << di.num_uses
+      << " last_use_pos: " << di.last_use_pos
+      << " used_in_another_construct: " << (di.used_in_another_construct ? "true" : "false")
       << " requires_named_const_def: " << (di.requires_named_const_def ? "true" : "false")
       << " requires_hoisted_def: " << (di.requires_hoisted_def ? "true" : "false") << " phi_var: '"
       << di.phi_var << "'";
@@ -840,7 +851,7 @@ class FunctionEmitter {
         return info && info->pos != kInvalidBlockPos;
     }
 
-    /// Gets the local definition info for a result ID.
+    /// Gets the definition info for a result ID.
     /// @param id the SPIR-V ID of local definition.
     /// @returns the definition info for the given ID, if it exists, or nullptr
     DefInfo* GetDefInfo(uint32_t id) const {
@@ -1274,7 +1285,9 @@ class FunctionEmitter {
     // Mapping from block ID to its bookkeeping info.
     std::unordered_map<uint32_t, std::unique_ptr<BlockInfo>> block_info_;
 
-    // Mapping from a locally-defined result ID to its bookkeeping info.
+    // Mapping from a result ID to its bookkeeping info.  This may be
+    // either a result ID defined in the function body, or the ID of a
+    // module-scope variable.
     std::unordered_map<uint32_t, std::unique_ptr<DefInfo>> def_info_;
 
     // Structured constructs, where enclosing constructs precede their children.

src/tint/reader/spirv/parser_impl_module_var_test.cc

@@ -2537,6 +2537,68 @@ fn main(@builtin(vertex_index) x_1_param : u32) -> main_out {
     EXPECT_EQ(module_str, expected) << module_str;
 }
 
+TEST_F(SpvModuleScopeVarParserTest, VertexIndex_UsedTwice_DifferentConstructs) {
+    // Test crbug.com/tint/1577
+    // Builtin variables must not be hoisted. Before the fix, the reader
+    // would see two uses of the variable in different constructs and try
+    // to hoist it.  Only function-local definitions should be hoisted.
+    const std::string assembly = VertexIndexPreamble("%uint") + R"(
+    %bool = OpTypeBool
+    %900 = OpConstantTrue %bool
+    %main = OpFunction %void None %voidfn
+    %entry = OpLabel
+    %2 = OpLoad %uint %1   ; used in outer selection
+    OpSelectionMerge %99 None
+    OpBranchConditional %900 %30 %99
+
+    %30 = OpLabel
+    %3 = OpLoad %uint %1 ; used in inner selection
+    OpSelectionMerge %40 None
+    OpBranchConditional %900 %35 %40
+
+    %35 = OpLabel
+    OpBranch %40
+
+    %40 = OpLabel
+    OpBranch %99
+
+    %99 = OpLabel
+    OpReturn
+    OpFunctionEnd
+ )";
+    auto p = parser(test::Assemble(assembly));
+    ASSERT_TRUE(p->BuildAndParseInternalModule()) << p->error() << assembly;
+    EXPECT_TRUE(p->error().empty());
+    const auto module_str = test::ToString(p->program());
+    const std::string expected = R"(var<private> x_1 : u32;
+
+var<private> x_5 : vec4<f32>;
+
+fn main_1() {
+  let x_2 : u32 = x_1;
+  if (true) {
+    let x_3 : u32 = x_1;
+    if (true) {
+    }
+  }
+  return;
+}
+
+struct main_out {
+  @builtin(position)
+  x_5_1 : vec4<f32>,
+}
+
+@vertex
+fn main(@builtin(vertex_index) x_1_param : u32) -> main_out {
+  x_1 = x_1_param;
+  main_1();
+  return main_out(x_5);
+}
+)";
+    EXPECT_EQ(module_str, expected) << module_str;
+}
+
 TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_Load_CopyObject) {
     const std::string assembly = VertexIndexPreamble("%int") + R"(
     %main = OpFunction %void None %voidfn