spirv-reader: ignore PointSize builtin declared at module-scope
Fixed: tint:434
Change-Id: Ia29d0f7227e838fc5f9dd4ca521b5fd6b9a88637
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/36761
Auto-Submit: David Neto <dneto@google.com>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index b72b55a..9c78615 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -976,6 +976,9 @@
return false;
}
+ if (!RegisterIgnoredBuiltInVariables()) {
+ return false;
+ }
if (!RegisterLocallyDefinedValues()) {
return false;
}
@@ -2880,6 +2883,9 @@
}
bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) {
+ if (failed()) {
+ return false;
+ }
const auto result_id = inst.result_id();
const auto type_id = inst.type_id();
@@ -2994,14 +3000,9 @@
// a new named constant definition.
auto value_id = inst.GetSingleWordInOperand(0);
const auto skip = GetSkipReason(value_id);
- switch (skip) {
- case SkipReason::kDontSkip:
- break;
- case SkipReason::kOpaqueObject:
- case SkipReason::kPointSizeBuiltinPointer:
- case SkipReason::kPointSizeBuiltinValue:
- GetDefInfo(inst.result_id())->skip = skip;
- return true;
+ if (skip != SkipReason::kDontSkip) {
+ GetDefInfo(inst.result_id())->skip = skip;
+ return true;
}
auto expr = MakeExpression(value_id);
expr.type = RemapStorageClass(expr.type, result_id);
@@ -3235,6 +3236,14 @@
return {};
}
+ const auto base_id = inst.GetSingleWordInOperand(0);
+ const auto base_skip = GetSkipReason(base_id);
+ if (base_skip != SkipReason::kDontSkip) {
+ // This can occur for AccessChain with no indices.
+ GetDefInfo(inst.result_id())->skip = base_skip;
+ 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
@@ -3242,7 +3251,6 @@
TypedExpression current_expr(MakeOperand(inst, 0));
const auto constants = constant_mgr_->GetOperandConstants(&inst);
- const auto base_id = inst.GetSingleWordInOperand(0);
auto ptr_ty_id = def_use_mgr_->GetDef(base_id)->type_id();
uint32_t first_index = 1;
const auto num_in_operands = inst.NumInOperands();
@@ -3592,9 +3600,29 @@
create<ast::TypeConstructorExpression>(source, result_type, values)};
}
+bool FunctionEmitter::RegisterIgnoredBuiltInVariables() {
+ size_t index = def_info_.size();
+ for (auto& ignored_var : parser_impl_.ignored_builtins()) {
+ const auto id = ignored_var.first;
+ const auto builtin = ignored_var.second;
+ def_info_[id] =
+ std::make_unique<DefInfo>(*(def_use_mgr_->GetDef(id)), 0, index);
+ ++index;
+ auto& def = def_info_[id];
+ switch (builtin) {
+ case SpvBuiltInPointSize:
+ def->skip = SkipReason::kPointSizeBuiltinPointer;
+ break;
+ default:
+ return Fail() << "unrecognized ignored builtin: " << int(builtin);
+ }
+ }
+ return true;
+}
+
bool FunctionEmitter::RegisterLocallyDefinedValues() {
// Create a DefInfo for each value definition in this function.
- size_t index = 0;
+ size_t index = def_info_.size();
for (auto block_id : block_order_) {
const auto* block_info = GetBlockInfo(block_id);
const auto block_pos = block_info->pos;
@@ -3604,7 +3632,7 @@
continue;
}
def_info_[result_id] = std::make_unique<DefInfo>(inst, block_pos, index);
- index++;
+ ++index;
auto& info = def_info_[result_id];
// Determine storage class for pointer values. Do this in order because
diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h
index c27cde5..9002ea8 100644
--- a/src/reader/spirv/function.h
+++ b/src/reader/spirv/function.h
@@ -223,10 +223,13 @@
kPointSizeBuiltinValue,
};
-/// Bookkeeping info for a SPIR-V ID defined in the function.
-/// This will be valid for result IDs for:
-/// - instructions that are not OpLabel, and not OpFunctionParameter
-/// - are defined in a basic block visited in the block-order for the function.
+/// Bookkeeping info for a SPIR-V ID defined in the function, or some
+/// module-scope variables. This will be valid for result IDs that are:
+/// - defined in the function and:
+/// - instructions that are not OpLabel, and not OpFunctionParameter
+/// - are defined in a basic block visited in the block-order for the
+/// function.
+/// - certain module-scope builtin variables.
struct DefInfo {
/// Constructor.
/// @param def_inst the SPIR-V instruction defining the ID
@@ -240,8 +243,11 @@
/// The SPIR-V instruction that defines the ID.
const spvtools::opt::Instruction& inst;
- /// The position of the block that defines this ID, in the function block
- /// order. See method `FunctionEmitter::ComputeBlockOrderAndPositions`
+ /// 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
+ /// containing the definition of the ID.
+ /// See method `FunctionEmitter::ComputeBlockOrderAndPositions`
const uint32_t block_pos = 0;
/// An index for uniquely and deterministically ordering all DefInfo records
@@ -290,8 +296,8 @@
/// This is kNone for non-pointers.
ast::StorageClass storage_class = ast::StorageClass::kNone;
- /// The reason, if any, that this value should not be generated.
- /// Normally all values are generated. This field can be updated while
+ /// The reason, if any, that this value should be ignored.
+ /// Normally no values are ignored. This field can be updated while
/// generating code because sometimes we only discover necessary facts
/// in the middle of generating code.
SkipReason skip = SkipReason::kDontSkip;
@@ -456,8 +462,14 @@
/// @returns false if bad nesting has been detected.
bool FindIfSelectionInternalHeaders();
+ /// Creates a DefInfo record for each module-scope builtin variable
+ /// that should be ignored.
+ /// Populates the `def_info_` mapping for such IDs.
+ /// @returns false on failure
+ bool RegisterIgnoredBuiltInVariables();
+
/// Creates a DefInfo record for each locally defined SPIR-V ID.
- /// Populates the `def_info_` mapping with basic results.
+ /// Populates the `def_info_` mapping with basic results for such IDs.
/// @returns false on failure
bool RegisterLocallyDefinedValues();
diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc
index 3cb8646..ee91183 100644
--- a/src/reader/spirv/parser_impl.cc
+++ b/src/reader/spirv/parser_impl.cc
@@ -1092,8 +1092,10 @@
auto* ast_var =
MakeVariable(inst.result_id(), ast::StorageClass::kNone, ast_type,
true, ast_expr, std::move(spec_id_decos));
- ast_module_.AddGlobalVariable(ast_var);
- scalar_spec_constants_.insert(inst.result_id());
+ if (ast_var) {
+ ast_module_.AddGlobalVariable(ast_var);
+ scalar_spec_constants_.insert(inst.result_id());
+ }
}
}
return success_;
@@ -1209,7 +1211,9 @@
MakeVariable(var.result_id(), ast_storage_class, ast_store_type, false,
ast_constructor, ast::VariableDecorationList{});
// TODO(dneto): initializers (a.k.a. constructor expression)
- ast_module_.AddGlobalVariable(ast_var);
+ if (ast_var) {
+ ast_module_.AddGlobalVariable(ast_var);
+ }
}
// Emit gl_Position instead of gl_PerVertex
@@ -1261,8 +1265,15 @@
<< ": has no operand";
return nullptr;
}
- auto ast_builtin =
- enum_converter_.ToBuiltin(static_cast<SpvBuiltIn>(deco[1]));
+ const auto spv_builtin = static_cast<SpvBuiltIn>(deco[1]);
+ switch (spv_builtin) {
+ case SpvBuiltInPointSize:
+ ignored_builtins_[id] = spv_builtin;
+ return nullptr;
+ default:
+ break;
+ }
+ auto ast_builtin = enum_converter_.ToBuiltin(spv_builtin);
if (ast_builtin == ast::Builtin::kNone) {
return nullptr;
}
diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h
index b34177c..579d681 100644
--- a/src/reader/spirv/parser_impl.h
+++ b/src/reader/spirv/parser_impl.h
@@ -287,14 +287,15 @@
bool EmitFunction(const spvtools::opt::Function& f);
/// Creates an AST Variable node for a SPIR-V ID, including any attached
- /// decorations.
+ /// decorations, unless it's an ignorable builtin variable.
/// @param id the SPIR-V result ID
/// @param sc the storage class, which cannot be ast::StorageClass::kNone
/// @param type the type
/// @param is_const if true, the variable is const
/// @param constructor the variable constructor
/// @param decorations the variable decorations
- /// @returns a new Variable node, or null in the error case
+ /// @returns a new Variable node, or null in the ignorable variable case and
+ /// in the error case
ast::Variable* MakeVariable(uint32_t id,
ast::StorageClass sc,
ast::type::Type* type,
@@ -473,6 +474,9 @@
/// @returns the instruction, or nullptr on error
const spvtools::opt::Instruction* GetInstructionForTest(uint32_t id) const;
+ using BuiltInsMap = std::unordered_map<uint32_t, SpvBuiltIn>;
+ const BuiltInsMap& ignored_builtins() const { return ignored_builtins_; }
+
private:
/// Converts a specific SPIR-V type to a Tint type. Integer case
ast::type::Type* ConvertType(const spvtools::opt::analysis::Integer* int_ty);
@@ -626,6 +630,10 @@
// The inferred pointer type for the given handle variable.
std::unordered_map<const spvtools::opt::Instruction*, ast::type::Pointer*>
handle_type_;
+
+ /// Maps the SPIR-V ID of a module-scope builtin variable that should be
+ /// ignored, to its builtin kind.
+ BuiltInsMap ignored_builtins_;
};
} // namespace spirv
diff --git a/src/reader/spirv/parser_impl_module_var_test.cc b/src/reader/spirv/parser_impl_module_var_test.cc
index 1c568eb..b39f219 100644
--- a/src/reader/spirv/parser_impl_module_var_test.cc
+++ b/src/reader/spirv/parser_impl_module_var_test.cc
@@ -629,6 +629,160 @@
)") << module_str;
}
+std::string LoosePointSizePreamble() {
+ return R"(
+ OpCapability Shader
+ OpCapability Linkage ; so we don't have to declare an entry point
+ OpMemoryModel Logical Simple
+
+ OpDecorate %1 BuiltIn PointSize
+ %void = OpTypeVoid
+ %voidfn = OpTypeFunction %void
+ %float = OpTypeFloat 32
+ %uint = OpTypeInt 32 0
+ %uint_0 = OpConstant %uint 0
+ %uint_1 = OpConstant %uint 1
+ %11 = OpTypePointer Output %float
+ %1 = OpVariable %11 Output
+)";
+}
+
+TEST_F(SpvModuleScopeVarParserTest, BuiltinPointSize_Loose_Write1_IsErased) {
+ const std::string assembly = LoosePointSizePreamble() + R"(
+ %ptr = OpTypePointer Output %float
+ %one = OpConstant %float 1.0
+
+ %500 = OpFunction %void None %voidfn
+ %entry = OpLabel
+ OpStore %1 %one
+ OpReturn
+ OpFunctionEnd
+ )";
+ auto p = parser(test::Assemble(assembly));
+ EXPECT_TRUE(p->BuildAndParseInternalModule());
+ EXPECT_TRUE(p->error().empty());
+ const auto module_str =
+ Demangler().Demangle(p->get_module(), p->get_module().to_str());
+ EXPECT_EQ(module_str, R"(Module{
+ Function x_500 -> __void
+ ()
+ {
+ Return{}
+ }
+}
+)") << module_str;
+}
+
+TEST_F(SpvModuleScopeVarParserTest, BuiltinPointSize_Loose_WriteNon1_IsError) {
+ const std::string assembly = LoosePointSizePreamble() + R"(
+ %ptr = OpTypePointer Output %float
+ %999 = OpConstant %float 2.0
+
+ %500 = OpFunction %void None %voidfn
+ %entry = OpLabel
+ OpStore %1 %999
+ OpReturn
+ OpFunctionEnd
+ )";
+ auto p = parser(test::Assemble(assembly));
+ EXPECT_FALSE(p->BuildAndParseInternalModule());
+ EXPECT_THAT(p->error(),
+ HasSubstr("cannot store a value other than constant 1.0 to "
+ "PointSize builtin: OpStore %1 %999"));
+}
+
+TEST_F(SpvModuleScopeVarParserTest, BuiltinPointSize_Loose_ReadReplaced) {
+ const std::string assembly = LoosePointSizePreamble() + R"(
+ %ptr = OpTypePointer Private %float
+ %900 = OpVariable %ptr Private
+
+ %500 = OpFunction %void None %voidfn
+ %entry = OpLabel
+ %99 = OpLoad %float %1
+ OpStore %900 %99
+ OpReturn
+ OpFunctionEnd
+ )";
+ auto p = parser(test::Assemble(assembly));
+ EXPECT_TRUE(p->BuildAndParseInternalModule());
+ EXPECT_TRUE(p->error().empty());
+ const auto module_str =
+ Demangler().Demangle(p->get_module(), p->get_module().to_str());
+ EXPECT_EQ(module_str, R"(Module{
+ Variable{
+ x_900
+ private
+ __f32
+ }
+ Function x_500 -> __void
+ ()
+ {
+ Assignment{
+ Identifier[not set]{x_900}
+ ScalarConstructor[not set]{1.000000}
+ }
+ Return{}
+ }
+}
+)") << module_str;
+}
+
+TEST_F(SpvModuleScopeVarParserTest,
+ BuiltinPointSize_Loose_WriteViaCopyObjectPriorAccess_Erased) {
+ const std::string assembly = LoosePointSizePreamble() + R"(
+ %one = OpConstant %float 1.0
+
+ %500 = OpFunction %void None %voidfn
+ %entry = OpLabel
+ %20 = OpCopyObject %11 %1
+ %100 = OpAccessChain %11 %20
+ OpStore %100 %one
+ OpReturn
+ OpFunctionEnd
+ )";
+ auto p = parser(test::Assemble(assembly));
+ EXPECT_TRUE(p->BuildAndParseInternalModule()) << p->error();
+ EXPECT_TRUE(p->error().empty());
+ const auto module_str =
+ Demangler().Demangle(p->get_module(), p->get_module().to_str());
+ EXPECT_EQ(module_str, R"(Module{
+ Function x_500 -> __void
+ ()
+ {
+ Return{}
+ }
+}
+)") << module_str;
+}
+
+TEST_F(SpvModuleScopeVarParserTest,
+ BuiltinPointSize_Loose_WriteViaCopyObjectPostAccessChainErased) {
+ const std::string assembly = LoosePointSizePreamble() + R"(
+ %one = OpConstant %float 1.0
+
+ %500 = OpFunction %void None %voidfn
+ %entry = OpLabel
+ %100 = OpAccessChain %11 %1
+ %101 = OpCopyObject %11 %100
+ OpStore %101 %one
+ OpReturn
+ OpFunctionEnd
+ )";
+ auto p = parser(test::Assemble(assembly));
+ EXPECT_TRUE(p->BuildAndParseInternalModule()) << p->error();
+ EXPECT_TRUE(p->error().empty()) << p->error();
+ const auto module_str =
+ Demangler().Demangle(p->get_module(), p->get_module().to_str());
+ EXPECT_EQ(module_str, R"(Module{
+ Function x_500 -> __void
+ ()
+ {
+ Return{}
+ }
+}
+)") << module_str;
+}
+
TEST_F(SpvModuleScopeVarParserTest, BuiltinClipDistance_NotSupported) {
const std::string assembly = PerVertexPreamble() + R"(
%ptr_float = OpTypePointer Output %float
