reader/spirv: Don't create disjoint AST nodes

This is a waste of memory, and now fires a TINT_ASSERT() in the resolver.

Add some more information to the resolver assertion message so that its easier to identify the node. This is especially useful when there's no source information available.

Fixed: tint:740
Bug: tint:469
Change-Id: I0cd4529db7b3906e64da6ed7290163509eb0c3f2
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48689
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: David Neto <dneto@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>

src/reader/spirv/function.cc

@@ -3450,17 +3450,12 @@ TypedExpression FunctionEmitter::MakeAccessChain(
     return {};
   }
 
-  // A SPIR-V access chain is a single instruction with multiple indices
-  // walking down into composites.  The Tint AST represents this as
-  // ever-deeper nested indexing expressions. Start off with an expression
-  // for the base, and then bury that inside nested indexing expressions.
-  TypedExpression current_expr(MakeOperand(inst, 0));
-  const auto constants = constant_mgr_->GetOperandConstants(&inst);
-
   auto ptr_ty_id = def_use_mgr_->GetDef(base_id)->type_id();
   uint32_t first_index = 1;
   const auto num_in_operands = inst.NumInOperands();
 
+  TypedExpression current_expr;
+
   // If the variable was originally gl_PerVertex, then in the AST we
   // have instead emitted a gl_Position variable.
   // If computing the pointer to the Position builtin, then emit the
@@ -3526,6 +3521,15 @@ TypedExpression FunctionEmitter::MakeAccessChain(
     }
   }
 
+  // A SPIR-V access chain is a single instruction with multiple indices
+  // walking down into composites.  The Tint AST represents this as
+  // ever-deeper nested indexing expressions. Start off with an expression
+  // for the base, and then bury that inside nested indexing expressions.
+  if (!current_expr.expr) {
+    current_expr = MakeOperand(inst, 0);
+  }
+  const auto constants = constant_mgr_->GetOperandConstants(&inst);
+
   const auto* ptr_type_inst = def_use_mgr_->GetDef(ptr_ty_id);
   if (!ptr_type_inst || (ptr_type_inst->opcode() != SpvOpTypePointer)) {
     Fail() << "Access chain %" << inst.result_id()

src/reader/spirv/parser_impl.cc

@@ -845,15 +845,13 @@ sem::Type* ParserImpl::ConvertType(
     const spvtools::opt::analysis::Struct* struct_ty) {
   // Compute the struct decoration.
   auto struct_decorations = this->GetDecorationsFor(type_id);
-  ast::DecorationList ast_struct_decorations;
+  bool is_block_decorated = false;
   if (struct_decorations.size() == 1) {
     const auto decoration = struct_decorations[0][0];
     if (decoration == SpvDecorationBlock) {
-      ast_struct_decorations.push_back(
+      is_block_decorated = true;
-          create<ast::StructBlockDecoration>(Source{}));
     } else if (decoration == SpvDecorationBufferBlock) {
-      ast_struct_decorations.push_back(
+      is_block_decorated = true;
-          create<ast::StructBlockDecoration>(Source{}));
       remap_buffer_block_type_.insert(type_id);
     } else {
       Fail() << "struct with ID " << type_id
@@ -869,7 +867,6 @@ sem::Type* ParserImpl::ConvertType(
   ast::StructMemberList ast_members;
   const auto members = struct_ty->element_types();
   unsigned num_non_writable_members = 0;
-  bool is_per_vertex_struct = false;
   for (uint32_t member_index = 0; member_index < members.size();
        ++member_index) {
     const auto member_type_id = type_mgr_->GetId(members[member_index]);
@@ -878,8 +875,10 @@ sem::Type* ParserImpl::ConvertType(
       // Already emitted diagnostics.
       return nullptr;
     }
-    ast::DecorationList ast_member_decorations;
+
-    bool is_non_writable = false;
+    // Scan member for built-in decorations. Some vertex built-ins are handled
+    // specially, and should not generate a structure member.
+    bool create_ast_member = true;
     for (auto& decoration : GetDecorationsForMember(type_id, member_index)) {
       if (decoration.empty()) {
         Fail() << "malformed SPIR-V decoration: it's empty";
@@ -892,23 +891,31 @@ sem::Type* ParserImpl::ConvertType(
             builtin_position_.struct_type_id = type_id;
             builtin_position_.position_member_index = member_index;
             builtin_position_.position_member_type_id = member_type_id;
-            // Don't map the struct type.  But this is not an error either.
+            create_ast_member = false;  // Not part of the WGSL structure.
-            is_per_vertex_struct = true;
             break;
           case SpvBuiltInPointSize:  // not supported in WGSL, but ignore
             builtin_position_.pointsize_member_index = member_index;
-            is_per_vertex_struct = true;
+            create_ast_member = false;  // Not part of the WGSL structure.
             break;
           case SpvBuiltInClipDistance:  // not supported in WGSL
           case SpvBuiltInCullDistance:  // not supported in WGSL
-            // Silently ignore, so we can detect Position and PointSize
+            create_ast_member = false;  // Not part of the WGSL structure.
-            is_per_vertex_struct = true;
             break;
           default:
             Fail() << "unrecognized builtin " << decoration[1];
             return nullptr;
         }
-      } else if (decoration[0] == SpvDecorationNonWritable) {
+      }
+    }
+    if (!create_ast_member) {
+      // This member is decorated as a built-in, and is handled specially.
+      continue;
+    }
+
+    bool is_non_writable = false;
+    ast::DecorationList ast_member_decorations;
+    for (auto& decoration : GetDecorationsForMember(type_id, member_index)) {
+      if (decoration[0] == SpvDecorationNonWritable) {
         // WGSL doesn't represent individual members as non-writable. Instead,
         // apply the ReadOnly access control to the containing struct if all
         // the members are non-writable.
@@ -924,6 +931,7 @@ sem::Type* ParserImpl::ConvertType(
         }
       }
     }
+
     if (is_non_writable) {
       // Count a member as non-writable only once, no matter how many
       // NonWritable decorations are applied to it.
@@ -935,8 +943,9 @@ sem::Type* ParserImpl::ConvertType(
         std::move(ast_member_decorations));
     ast_members.push_back(ast_struct_member);
   }
-  if (is_per_vertex_struct) {
+
-    // We're replacing it by the Position builtin alone.
+  if (ast_members.empty()) {
+    // All members were likely built-ins. Don't generate an empty AST structure.
     return nullptr;
   }
 
@@ -946,6 +955,11 @@ sem::Type* ParserImpl::ConvertType(
 
   // Now make the struct.
   auto sym = builder_.Symbols().Register(name);
+  ast::DecorationList ast_struct_decorations;
+  if (is_block_decorated) {
+    ast_struct_decorations.emplace_back(
+        create<ast::StructBlockDecoration>(Source{}));
+  }
   auto* ast_struct = create<ast::Struct>(Source{}, sym, std::move(ast_members),
                                          std::move(ast_struct_decorations));
   auto* result = builder_.create<sem::StructType>(ast_struct);

src/resolver/resolver.cc

@@ -272,7 +272,8 @@ bool Resolver::ResolveInternal() {
       }
       TINT_ICE(diagnostics_) << "AST node '" << node->TypeInfo().name
                              << "' was not reached by the resolver\n"
-                             << "At: " << node->source();
+                             << "At: " << node->source() << "\n"
+                             << "Content: " << builder_->str(node);
     }
   }