tint/writer: Replace scope_stack_, fix type ctor scoping.

ScopeStack is not needed here - the resolver already provides variable scoping with the sem::Variables.

Re-purpose scope_stack_ for a stack of Scope, which now holds the type constructor -> SPIR-V ID map.
This map needs to be per-scope, to fix issues like crbug.com/tint/1520

Fixed: tint:1520
Change-Id: Ifa7749338abf63652a1369e76cf5400be1c37298
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/88301
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>

src/tint/writer/spirv/builder.cc

@@ -245,7 +245,7 @@ Builder::AccessorInfo::~AccessorInfo() {}
 
 Builder::Builder(const Program* program, bool zero_initialize_workgroup_memory)
     : builder_(ProgramBuilder::Wrap(program)),
-      scope_stack_({}),
+      scope_stack_{Scope{}},
       zero_initialize_workgroup_memory_(zero_initialize_workgroup_memory) {}
 
 Builder::~Builder() = default;
@@ -279,6 +279,30 @@ bool Builder::Build() {
   return true;
 }
 
+void Builder::RegisterVariable(const sem::Variable* var, uint32_t id) {
+  var_to_id_.emplace(var, id);
+  id_to_var_.emplace(id, var);
+}
+
+uint32_t Builder::LookupVariableID(const sem::Variable* var) {
+  auto it = var_to_id_.find(var);
+  if (it == var_to_id_.end()) {
+    error_ = "unable to find ID for variable: " +
+             builder_.Symbols().NameFor(var->Declaration()->symbol);
+    return 0;
+  }
+  return it->second;
+}
+
+void Builder::PushScope() {
+  // Push a new scope, by copying the top-most stack
+  scope_stack_.push_back(scope_stack_.back());
+}
+
+void Builder::PopScope() {
+  scope_stack_.pop_back();
+}
+
 Operand Builder::result_op() {
   return Operand::Int(next_id());
 }
@@ -441,7 +465,7 @@ bool Builder::GenerateEntryPoint(const ast::Function* func, uint32_t id) {
       continue;
     }
 
-    uint32_t var_id = scope_stack_.Get(var->Declaration()->symbol);
+    uint32_t var_id = LookupVariableID(var);
     if (var_id == 0) {
       error_ = "unable to find ID for global variable: " +
                builder_.Symbols().NameFor(var->Declaration()->symbol);
@@ -588,8 +612,8 @@ bool Builder::GenerateFunction(const ast::Function* func_ast) {
     return false;
   }
 
-  scope_stack_.Push();
-  TINT_DEFER(scope_stack_.Pop());
+  PushScope();
+  TINT_DEFER(PopScope());
 
   auto definition_inst = Instruction{
       spv::Op::OpFunction,
@@ -612,7 +636,7 @@ bool Builder::GenerateFunction(const ast::Function* func_ast) {
     params.push_back(Instruction{spv::Op::OpFunctionParameter,
                                  {Operand::Int(param_type_id), param_op}});
 
-    scope_stack_.Set(param->Declaration()->symbol, param_id);
+    RegisterVariable(param, param_id);
   }
 
   push_function(Function{definition_inst, result_op(), std::move(params)});
@@ -680,20 +704,21 @@ bool Builder::GenerateFunctionVariable(const ast::Variable* var) {
     }
   }
 
+  auto* sem = builder_.Sem().Get(var);
+
   if (var->is_const) {
     if (!var->constructor) {
       error_ = "missing constructor for constant";
       return false;
     }
-    scope_stack_.Set(var->symbol, init_id);
-    spirv_id_to_variable_[init_id] = var;
+    RegisterVariable(sem, init_id);
     return true;
   }
 
   auto result = result_op();
   auto var_id = result.to_i();
   auto sc = ast::StorageClass::kFunction;
-  auto* type = builder_.Sem().Get(var)->Type();
+  auto* type = sem->Type();
   auto type_id = GenerateTypeIfNeeded(type);
   if (type_id == 0) {
     return false;
@@ -719,8 +744,7 @@ bool Builder::GenerateFunctionVariable(const ast::Variable* var) {
     }
   }
 
-  scope_stack_.Set(var->symbol, var_id);
-  spirv_id_to_variable_[var_id] = var;
+  RegisterVariable(sem, var_id);
 
   return true;
 }
@@ -775,8 +799,7 @@ bool Builder::GenerateGlobalVariable(const ast::Variable* var) {
                {Operand::Int(init_id),
                 Operand::String(builder_.Symbols().NameFor(var->symbol))});
 
-    scope_stack_.Set(var->symbol, init_id);
-    spirv_id_to_variable_[init_id] = var;
+    RegisterVariable(sem, init_id);
     return true;
   }
 
@@ -898,8 +921,7 @@ bool Builder::GenerateGlobalVariable(const ast::Variable* var) {
     }
   }
 
-  scope_stack_.Set(var->symbol, var_id);
-  spirv_id_to_variable_[var_id] = var;
+  RegisterVariable(sem, var_id);
   return true;
 }
 
@@ -1174,12 +1196,13 @@ uint32_t Builder::GenerateAccessorExpression(const ast::Expression* expr) {
 
 uint32_t Builder::GenerateIdentifierExpression(
     const ast::IdentifierExpression* expr) {
-  uint32_t val = scope_stack_.Get(expr->symbol);
-  if (val == 0) {
-    error_ = "unable to find variable with identifier: " +
-             builder_.Symbols().NameFor(expr->symbol);
+  auto* sem = builder_.Sem().Get(expr);
+  if (auto* user = sem->As<sem::VariableUser>()) {
+    return LookupVariableID(user->Variable());
   }
-  return val;
+  error_ = "identifier '" + builder_.Symbols().NameFor(expr->symbol) +
+           "' does not resolve to a variable";
+  return 0;
 }
 
 uint32_t Builder::GenerateExpressionWithLoadIfNeeded(
@@ -1479,8 +1502,12 @@ uint32_t Builder::GenerateTypeConstructorOrConversion(
     }
   }
 
+  auto& stack = (result_is_spec_composite || result_is_constant_composite)
+                    ? scope_stack_[0]       // Global scope
+                    : scope_stack_.back();  // Lexical scope
+
   return utils::GetOrCreate(
-      type_constructor_to_id_, OperandListKey{ops}, [&]() -> uint32_t {
+      stack.type_ctor_to_id_, OperandListKey{ops}, [&]() -> uint32_t {
         auto result = result_op();
         ops[kOpsResultIdx] = result;
 
@@ -2193,8 +2220,8 @@ uint32_t Builder::GenerateBinaryExpression(const ast::BinaryExpression* expr) {
 }
 
 bool Builder::GenerateBlockStatement(const ast::BlockStatement* stmt) {
-  scope_stack_.Push();
-  TINT_DEFER(scope_stack_.Pop());
+  PushScope();
+  TINT_DEFER(PopScope());
   return GenerateBlockStatementWithoutScoping(stmt);
 }
 
@@ -3653,8 +3680,8 @@ bool Builder::GenerateLoopStatement(const ast::LoopStatement* stmt) {
   // We need variables from the body to be visible in the continuing block, so
   // manage scope outside of GenerateBlockStatement.
   {
-    scope_stack_.Push();
-    TINT_DEFER(scope_stack_.Pop());
+    PushScope();
+    TINT_DEFER(PopScope());
 
     if (!GenerateBlockStatementWithoutScoping(stmt->body)) {
       return false;

src/tint/writer/spirv/builder.h

@@ -584,6 +584,22 @@ class Builder {
   uint32_t GenerateConstantVectorSplatIfNeeded(const sem::Vector* type,
                                                uint32_t value_id);
 
+  /// Registers the semantic variable to the given SPIR-V ID
+  /// @param var the semantic variable
+  /// @param id the generated SPIR-V identifier for the variable
+  void RegisterVariable(const sem::Variable* var, uint32_t id);
+
+  /// Looks up the SPIR-V ID for the variable, which must have been registered
+  /// with a call to RegisterVariable()
+  /// @returns the SPIR-V ID, or 0 if the variable was not found
+  uint32_t LookupVariableID(const sem::Variable* var);
+
+  /// Pushes a new scope
+  void PushScope();
+
+  /// Pops the top-most scope
+  void PopScope();
+
   ProgramBuilder builder_;
   std::string error_;
   uint32_t next_id_ = 1;
@@ -599,18 +615,23 @@ class Builder {
   InstructionList annotations_;
   std::vector<Function> functions_;
 
+  // Scope holds per-block information
+  struct Scope {
+    std::unordered_map<OperandListKey, uint32_t> type_ctor_to_id_;
+  };
+
+  std::unordered_map<const sem::Variable*, uint32_t> var_to_id_;
+  std::unordered_map<uint32_t, const sem::Variable*> id_to_var_;
   std::unordered_map<std::string, uint32_t> import_name_to_id_;
   std::unordered_map<Symbol, uint32_t> func_symbol_to_id_;
   std::unordered_map<sem::CallTargetSignature, uint32_t> func_sig_to_id_;
   std::unordered_map<const sem::Type*, uint32_t> type_to_id_;
   std::unordered_map<ScalarConstant, uint32_t> const_to_id_;
-  std::unordered_map<OperandListKey, uint32_t> type_constructor_to_id_;
   std::unordered_map<const sem::Type*, uint32_t> const_null_to_id_;
   std::unordered_map<uint64_t, uint32_t> const_splat_to_id_;
   std::unordered_map<const sem::Type*, uint32_t>
       texture_type_to_sampled_image_type_id_;
-  ScopeStack<uint32_t> scope_stack_;
-  std::unordered_map<uint32_t, const ast::Variable*> spirv_id_to_variable_;
+  std::vector<Scope> scope_stack_;
   std::vector<uint32_t> merge_stack_;
   std::vector<uint32_t> continue_stack_;
   std::unordered_set<uint32_t> capability_set_;

src/tint/writer/spirv/builder_constructor_expression_test.cc

@@ -1850,5 +1850,99 @@ TEST_F(SpvBuilderConstructorTest,
   EXPECT_FALSE(b.has_error());
 }
 
+TEST_F(SpvBuilderConstructorTest, ConstantCompositeScoping) {
+  // if (true) {
+  //    let x = vec3<f32>(1.0, 2.0, 3.0);
+  // }
+  // let y = vec3<f32>(1.0, 2.0, 3.0); // Reuses the ID 'x'
+
+  WrapInFunction(
+      If(true, Block(Decl(Let("x", nullptr, vec3<f32>(1.f, 2.f, 3.f))))),
+      Decl(Let("y", nullptr, vec3<f32>(1.f, 2.f, 3.f))));
+
+  spirv::Builder& b = SanitizeAndBuild();
+  ASSERT_TRUE(b.Build());
+
+  EXPECT_EQ(DumpBuilder(b), R"(OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %3 "test_function"
+OpExecutionMode %3 LocalSize 1 1 1
+OpName %3 "test_function"
+%2 = OpTypeVoid
+%1 = OpTypeFunction %2
+%5 = OpTypeBool
+%6 = OpConstantTrue %5
+%10 = OpTypeFloat 32
+%9 = OpTypeVector %10 3
+%11 = OpConstant %10 1
+%12 = OpConstant %10 2
+%13 = OpConstant %10 3
+%14 = OpConstantComposite %9 %11 %12 %13
+%3 = OpFunction %2 None %1
+%4 = OpLabel
+OpSelectionMerge %7 None
+OpBranchConditional %6 %8 %7
+%8 = OpLabel
+OpBranch %7
+%7 = OpLabel
+OpReturn
+OpFunctionEnd
+)");
+  Validate(b);
+}
+
+// TODO(crbug.com/tint/1155) Implement when overrides are fully implemented.
+// TEST_F(SpvBuilderConstructorTest, SpecConstantCompositeScoping)
+
+TEST_F(SpvBuilderConstructorTest, CompositeConstructScoping) {
+  // var one = 1.0;
+  // if (true) {
+  //    let x = vec3<f32>(one, 2.0, 3.0);
+  // }
+  // let y = vec3<f32>(one, 2.0, 3.0); // Mustn't reuse the ID 'x'
+
+  WrapInFunction(
+      Decl(Var("one", nullptr, Expr(1.f))),
+      If(true, Block(Decl(Let("x", nullptr, vec3<f32>("one", 2.f, 3.f))))),
+      Decl(Let("y", nullptr, vec3<f32>("one", 2.f, 3.f))));
+
+  spirv::Builder& b = SanitizeAndBuild();
+  ASSERT_TRUE(b.Build());
+
+  EXPECT_EQ(DumpBuilder(b), R"(OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %3 "test_function"
+OpExecutionMode %3 LocalSize 1 1 1
+OpName %3 "test_function"
+OpName %7 "one"
+%2 = OpTypeVoid
+%1 = OpTypeFunction %2
+%5 = OpTypeFloat 32
+%6 = OpConstant %5 1
+%8 = OpTypePointer Function %5
+%9 = OpConstantNull %5
+%10 = OpTypeBool
+%11 = OpConstantTrue %10
+%14 = OpTypeVector %5 3
+%16 = OpConstant %5 2
+%17 = OpConstant %5 3
+%3 = OpFunction %2 None %1
+%4 = OpLabel
+%7 = OpVariable %8 Function %9
+OpStore %7 %6
+OpSelectionMerge %12 None
+OpBranchConditional %11 %13 %12
+%13 = OpLabel
+%15 = OpLoad %5 %7
+%18 = OpCompositeConstruct %14 %15 %16 %17
+OpBranch %12
+%12 = OpLabel
+%19 = OpLoad %5 %7
+%20 = OpCompositeConstruct %14 %19 %16 %17
+OpReturn
+OpFunctionEnd
+)");
+  Validate(b);
+}
 }  // namespace
 }  // namespace tint::writer::spirv