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>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index a6270c1..ea87c3d 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -3450,17 +3450,12 @@
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 @@
}
}
+ // 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()
diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc
index 336e18b..cfcf986 100644
--- a/src/reader/spirv/parser_impl.cc
+++ b/src/reader/spirv/parser_impl.cc
@@ -845,15 +845,13 @@
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(
- create<ast::StructBlockDecoration>(Source{}));
+ is_block_decorated = true;
} else if (decoration == SpvDecorationBufferBlock) {
- ast_struct_decorations.push_back(
- create<ast::StructBlockDecoration>(Source{}));
+ is_block_decorated = true;
remap_buffer_block_type_.insert(type_id);
} else {
Fail() << "struct with ID " << type_id
@@ -869,7 +867,6 @@
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 @@
// 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 @@
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.
- is_per_vertex_struct = true;
+ create_ast_member = false; // Not part of the WGSL structure.
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
- is_per_vertex_struct = true;
+ create_ast_member = false; // Not part of the WGSL structure.
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 @@
}
}
}
+
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 @@
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 @@
// 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);
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index 85501b0..ebb18c0 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -272,7 +272,8 @@
}
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);
}
}
