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>

src/reader/spirv/function.cc

@@ -976,6 +976,9 @@ bool FunctionEmitter::EmitBody() {
     return false;
   }
 
+  if (!RegisterIgnoredBuiltInVariables()) {
+    return false;
+  }
   if (!RegisterLocallyDefinedValues()) {
     return false;
   }
@@ -2880,6 +2883,9 @@ bool FunctionEmitter::EmitConstDefOrWriteToHoistedVar(
 }
 
 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,12 +3000,7 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) {
       // a new named constant definition.
       auto value_id = inst.GetSingleWordInOperand(0);
       const auto skip = GetSkipReason(value_id);
-      switch (skip) {
+      if (skip != SkipReason::kDontSkip) {
-        case SkipReason::kDontSkip:
-          break;
-        case SkipReason::kOpaqueObject:
-        case SkipReason::kPointSizeBuiltinPointer:
-        case SkipReason::kPointSizeBuiltinValue:
         GetDefInfo(inst.result_id())->skip = skip;
         return true;
       }
@@ -3235,6 +3236,14 @@ TypedExpression FunctionEmitter::MakeAccessChain(
     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 FunctionEmitter::MakeAccessChain(
   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 @@ TypedExpression FunctionEmitter::MakeVectorShuffle(
           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 @@ bool FunctionEmitter::RegisterLocallyDefinedValues() {
         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

src/reader/spirv/function.h

@@ -223,10 +223,13 @@ enum class SkipReason {
   kPointSizeBuiltinValue,
 };
 
-/// Bookkeeping info for a SPIR-V ID defined in the function.
+/// Bookkeeping info for a SPIR-V ID defined in the function, or some
-/// This will be valid for result IDs for:
+/// 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.
+///    - 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 @@ struct DefInfo {
 
   /// 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
+  /// The position of the first block in which this ID is visible, in function
-  /// order.  See method `FunctionEmitter::ComputeBlockOrderAndPositions`
+  /// 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 @@ struct DefInfo {
   /// This is kNone for non-pointers.
   ast::StorageClass storage_class = ast::StorageClass::kNone;
 
-  /// The reason, if any, that this value should not be generated.
+  /// The reason, if any, that this value should be ignored.
-  /// Normally all values are generated.  This field can be updated while
+  /// 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 @@ class FunctionEmitter {
   /// @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();
 

src/reader/spirv/parser_impl.cc

@@ -1092,10 +1092,12 @@ bool ParserImpl::EmitScalarSpecConstants() {
       auto* ast_var =
           MakeVariable(inst.result_id(), ast::StorageClass::kNone, ast_type,
                        true, ast_expr, std::move(spec_id_decos));
+      if (ast_var) {
         ast_module_.AddGlobalVariable(ast_var);
         scalar_spec_constants_.insert(inst.result_id());
       }
     }
+  }
   return success_;
 }
 
@@ -1209,8 +1211,10 @@ bool ParserImpl::EmitModuleScopeVariables() {
         MakeVariable(var.result_id(), ast_storage_class, ast_store_type, false,
                      ast_constructor, ast::VariableDecorationList{});
     // TODO(dneto): initializers (a.k.a. constructor expression)
+    if (ast_var) {
       ast_module_.AddGlobalVariable(ast_var);
     }
+  }
 
   // Emit gl_Position instead of gl_PerVertex
   if (builtin_position_.per_vertex_var_id) {
@@ -1261,8 +1265,15 @@ ast::Variable* ParserImpl::MakeVariable(
                << ": has no operand";
         return nullptr;
       }
-      auto ast_builtin =
+      const auto spv_builtin = static_cast<SpvBuiltIn>(deco[1]);
-          enum_converter_.ToBuiltin(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;
       }

src/reader/spirv/parser_impl.h

@@ -287,14 +287,15 @@ class ParserImpl : Reader {
   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 @@ class ParserImpl : Reader {
   /// @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 @@ class ParserImpl : Reader {
   // 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

src/reader/spirv/parser_impl_module_var_test.cc

@@ -629,6 +629,160 @@ TEST_F(SpvModuleScopeVarParserTest,
 )") << 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