spirv-reader: refactor handling pipeline IO decorations

This is in preparation for handling the "invariant" decoration.

Bug: tint:972
Change-Id: I17465946932ab37a32dfd3c477525649ab622c6f
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/58580
Auto-Submit: David Neto <dneto@google.com>
Commit-Queue: James Price <jrprice@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>

src/reader/spirv/function.cc

@@ -981,12 +981,10 @@ bool FunctionEmitter::EmitPipelineInput(std::string var_name,
     index_prefix.push_back(0);
     for (int i = 0; i < static_cast<int>(members.size()); ++i) {
       index_prefix.back() = i;
-      auto* location = parser_impl_.GetMemberLocation(*struct_type, i);
-      SetLocation(decos, location);
       ast::DecorationList member_decos(*decos);
-      if (!parser_impl_.ConvertInterpolationDecorations(
+      if (!parser_impl_.ConvertPipelineDecorations(
               struct_type,
-              parser_impl_.GetMemberInterpolationDecorations(*struct_type, i),
+              parser_impl_.GetMemberPipelineDecorations(*struct_type, i),
               &member_decos)) {
         return false;
       }
@@ -996,7 +994,7 @@ bool FunctionEmitter::EmitPipelineInput(std::string var_name,
         return false;
       }
       // Copy the location as updated by nested expansion of the member.
-      SetLocation(decos, GetLocation(member_decos));
+      parser_impl_.SetLocation(decos, GetLocation(member_decos));
     }
     return success();
   }
@@ -1064,26 +1062,6 @@ void FunctionEmitter::IncrementLocation(ast::DecorationList* decos) {
   }
 }
 
-ast::Decoration* FunctionEmitter::SetLocation(ast::DecorationList* decos,
-                                              ast::Decoration* replacement) {
-  if (!replacement) {
-    return nullptr;
-  }
-  for (auto*& deco : *decos) {
-    if (deco->Is<ast::LocationDecoration>()) {
-      // Replace this location decoration with the replacement.
-      // The old one doesn't leak because it's kept in the builder's AST node
-      // list.
-      ast::Decoration* result = deco;
-      deco = replacement;
-      return result;
-    }
-  }
-  // The list didn't have a location. Add it.
-  decos->push_back(replacement);
-  return nullptr;
-}
-
 ast::Decoration* FunctionEmitter::GetLocation(
     const ast::DecorationList& decos) {
   for (auto* const& deco : decos) {
@@ -1141,12 +1119,10 @@ bool FunctionEmitter::EmitPipelineOutput(std::string var_name,
     index_prefix.push_back(0);
     for (int i = 0; i < static_cast<int>(members.size()); ++i) {
       index_prefix.back() = i;
-      auto* location = parser_impl_.GetMemberLocation(*struct_type, i);
-      SetLocation(decos, location);
       ast::DecorationList member_decos(*decos);
-      if (!parser_impl_.ConvertInterpolationDecorations(
+      if (!parser_impl_.ConvertPipelineDecorations(
               struct_type,
-              parser_impl_.GetMemberInterpolationDecorations(*struct_type, i),
+              parser_impl_.GetMemberPipelineDecorations(*struct_type, i),
               &member_decos)) {
         return false;
       }
@@ -1156,7 +1132,7 @@ bool FunctionEmitter::EmitPipelineOutput(std::string var_name,
         return false;
       }
       // Copy the location as updated by nested expansion of the member.
-      SetLocation(decos, GetLocation(member_decos));
+      parser_impl_.SetLocation(decos, GetLocation(member_decos));
     }
     return success();
   }

src/reader/spirv/function.h

@@ -473,17 +473,6 @@ class FunctionEmitter {
   /// @param decos the decoration list to modify
   void IncrementLocation(ast::DecorationList* decos);
 
-  /// Updates the decoration list, placing a non-null location decoration into
-  /// the list, replacing an existing one if it exists. Does nothing if the
-  /// replacement is nullptr.
-  /// Assumes the list contains at most one Location decoration.
-  /// @param decos the decoration list to modify
-  /// @param replacement the location decoration to place into the list
-  /// @returns the location decoration that was replaced, if one was replaced,
-  /// or null otherwise.
-  ast::Decoration* SetLocation(ast::DecorationList* decos,
-                               ast::Decoration* replacement);
-
   /// Returns the Location dcoration, if it exists.
   /// @param decos the list of decorations to search
   /// @returns the Location decoration, or nullptr if it doesn't exist

src/reader/spirv/parser_impl.cc

@@ -234,12 +234,14 @@ bool AssumesResultSignednessMatchesFirstOperand(GLSLstd450 extended_opcode) {
 }
 
 // @param a SPIR-V decoration
-// @return true when the given decoration is an interpolation decoration.
+// @return true when the given decoration is a pipeline decoration other than a
-bool IsInterpolationDecoration(const Decoration& deco) {
+// bulitin variable.
+bool IsPipelineDecoration(const Decoration& deco) {
   if (deco.size() < 1) {
     return false;
   }
   switch (deco[0]) {
+    case SpvDecorationLocation:
     case SpvDecorationFlat:
     case SpvDecorationNoPerspective:
     case SpvDecorationCentroid:
@@ -1115,22 +1117,15 @@ const Type* ParserImpl::ConvertType(
     bool is_non_writable = false;
     ast::DecorationList ast_member_decorations;
     for (auto& decoration : GetDecorationsForMember(type_id, member_index)) {
-      switch (decoration[0]) {
+      if (IsPipelineDecoration(decoration)) {
-        case SpvDecorationNonWritable:
+        // IO decorations are handled when emitting the entry point.
-
+        continue;
+      } else 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.
         is_non_writable = true;
-          break;
+      } else {
-        case SpvDecorationLocation:
-        case SpvDecorationFlat:
-        case SpvDecorationNoPerspective:
-        case SpvDecorationCentroid:
-        case SpvDecorationSample:
-          // IO decorations are handled when emitting the entry point.
-          break;
-        default: {
         auto* ast_member_decoration =
             ConvertMemberDecoration(type_id, member_index, decoration);
         if (!success_) {
@@ -1139,8 +1134,6 @@ const Type* ParserImpl::ConvertType(
         if (ast_member_decoration) {
           ast_member_decorations.push_back(ast_member_decoration);
         }
-          break;
-        }
       }
     }
 
@@ -1622,7 +1615,7 @@ bool ParserImpl::ConvertDecorationsForVariable(uint32_t id,
                                                const Type** store_type,
                                                ast::DecorationList* decorations,
                                                bool transfer_pipeline_io) {
-  DecorationList interpolation_decorations;
+  DecorationList non_builtin_pipeline_decorations;
   for (auto& deco : GetDecorationsFor(id)) {
     if (deco.empty()) {
       return Fail() << "malformed decoration on ID " << id << ": it is empty";
@@ -1685,18 +1678,8 @@ bool ParserImpl::ConvertDecorationsForVariable(uint32_t id,
             create<ast::BuiltinDecoration>(Source{}, ast_builtin));
       }
     }
-    if (deco[0] == SpvDecorationLocation) {
+    if (transfer_pipeline_io && IsPipelineDecoration(deco)) {
-      if (deco.size() != 2) {
+      non_builtin_pipeline_decorations.push_back(deco);
-        return Fail() << "malformed Location decoration on ID " << id
-                      << ": requires one literal operand";
-      }
-      if (transfer_pipeline_io) {
-        decorations->emplace_back(
-            create<ast::LocationDecoration>(Source{}, deco[1]));
-      }
-    }
-    if (transfer_pipeline_io && IsInterpolationDecoration(deco)) {
-      interpolation_decorations.push_back(deco);
     }
     if (deco[0] == SpvDecorationDescriptorSet) {
       if (deco.size() == 1) {
@@ -1717,8 +1700,8 @@ bool ParserImpl::ConvertDecorationsForVariable(uint32_t id,
   }
 
   if (transfer_pipeline_io) {
-    if (!ConvertInterpolationDecorations(*store_type, interpolation_decorations,
+    if (!ConvertPipelineDecorations(
-                                         decorations)) {
+            *store_type, non_builtin_pipeline_decorations, decorations)) {
       return false;
     }
   }
@@ -1726,22 +1709,42 @@ bool ParserImpl::ConvertDecorationsForVariable(uint32_t id,
   return success();
 }
 
-DecorationList ParserImpl::GetMemberInterpolationDecorations(
+DecorationList ParserImpl::GetMemberPipelineDecorations(
     const Struct& struct_type,
     int member_index) {
   // Yes, I could have used std::copy_if or std::copy_if.
   DecorationList result;
   for (const auto& deco : GetDecorationsForMember(
            struct_id_for_symbol_[struct_type.name], member_index)) {
-    if (IsInterpolationDecoration(deco)) {
+    if (IsPipelineDecoration(deco)) {
       result.emplace_back(deco);
     }
   }
   return result;
 }
 
-bool ParserImpl::ConvertInterpolationDecorations(
+ast::Decoration* ParserImpl::SetLocation(ast::DecorationList* decos,
-    const Type* store_type,
+                                         ast::Decoration* replacement) {
+  if (!replacement) {
+    return nullptr;
+  }
+  for (auto*& deco : *decos) {
+    if (deco->Is<ast::LocationDecoration>()) {
+      // Replace this location decoration with the replacement.
+      // The old one doesn't leak because it's kept in the builder's AST node
+      // list.
+      ast::Decoration* result = nullptr;
+      result = deco;
+      deco = replacement;
+      return result;  // Assume there is only one such decoration.
+    }
+  }
+  // The list didn't have a location. Add it.
+  decos->push_back(replacement);
+  return nullptr;
+}
+
+bool ParserImpl::ConvertPipelineDecorations(const Type* store_type,
                                             const DecorationList& decorations,
                                             ast::DecorationList* ast_decos) {
   bool has_interpolate_no_perspective = false;
@@ -1749,7 +1752,17 @@ bool ParserImpl::ConvertInterpolationDecorations(
   bool has_interpolate_sampling_sample = false;
 
   for (const auto& deco : decorations) {
-    if (deco[0] == SpvDecorationFlat) {
+    TINT_ASSERT(Reader, deco.size() > 0);
+    switch (deco[0]) {
+      case SpvDecorationLocation:
+        if (deco.size() != 2) {
+          return Fail() << "malformed Location decoration on ID requires one "
+                           "literal operand";
+        }
+        SetLocation(ast_decos,
+                    create<ast::LocationDecoration>(Source{}, deco[1]));
+        break;
+      case SpvDecorationFlat:
         // In WGSL, integral types are always flat, and so the decoration
         // is never specified.
         if (!store_type->IsIntegerScalarOrVector()) {
@@ -1759,29 +1772,32 @@ bool ParserImpl::ConvertInterpolationDecorations(
           // Only one interpolate attribute is allowed.
           return true;
         }
-    }
+        break;
-    if (deco[0] == SpvDecorationNoPerspective) {
+      case SpvDecorationNoPerspective:
         if (store_type->IsIntegerScalarOrVector()) {
           // This doesn't capture the array or struct case.
           return Fail() << "NoPerspective is invalid on integral IO";
         }
         has_interpolate_no_perspective = true;
-    }
+        break;
-    if (deco[0] == SpvDecorationCentroid) {
+      case SpvDecorationCentroid:
         if (store_type->IsIntegerScalarOrVector()) {
           // This doesn't capture the array or struct case.
           return Fail()
                  << "Centroid interpolation sampling is invalid on integral IO";
         }
         has_interpolate_sampling_centroid = true;
-    }
+        break;
-    if (deco[0] == SpvDecorationSample) {
+      case SpvDecorationSample:
         if (store_type->IsIntegerScalarOrVector()) {
           // This doesn't capture the array or struct case.
           return Fail()
                  << "Sample interpolation sampling is invalid on integral IO";
         }
         has_interpolate_sampling_sample = true;
+        break;
+      default:
+        break;
     }
   }
 
@@ -2818,22 +2834,6 @@ std::string ParserImpl::GetMemberName(const Struct& struct_type,
   return namer_.GetMemberName(where->second, member_index);
 }
 
-ast::Decoration* ParserImpl::GetMemberLocation(const Struct& struct_type,
-                                               int member_index) {
-  auto where = struct_id_for_symbol_.find(struct_type.name);
-  if (where == struct_id_for_symbol_.end()) {
-    Fail() << "no structure type registered for symbol";
-    return nullptr;
-  }
-  const auto type_id = where->second;
-  for (auto& deco : GetDecorationsForMember(type_id, member_index)) {
-    if ((deco.size() == 2) && (deco[0] == SpvDecorationLocation)) {
-      return create<ast::LocationDecoration>(Source{}, deco[1]);
-    }
-  }
-  return nullptr;
-}
-
 WorkgroupSizeInfo::WorkgroupSizeInfo() = default;
 
 WorkgroupSizeInfo::~WorkgroupSizeInfo() = default;

src/reader/spirv/parser_impl.h

@@ -252,15 +252,26 @@ class ParserImpl : Reader {
                                      ast::DecorationList* ast_decos,
                                      bool transfer_pipeline_io);
 
-  /// Converts SPIR-V interpolation decorations into AST decorations.
+  /// Converts SPIR-V decorations for pipeline IO into AST decorations.
   /// @param store_type the store type for the variable or member
   /// @param decorations the SPIR-V interpolation decorations
   /// @param ast_decos the decoration list to populate.
   /// @returns false if conversion fails
-  bool ConvertInterpolationDecorations(const Type* store_type,
+  bool ConvertPipelineDecorations(const Type* store_type,
                                   const DecorationList& decorations,
                                   ast::DecorationList* ast_decos);
 
+  /// Updates the decoration list, placing a non-null location decoration into
+  /// the list, replacing an existing one if it exists. Does nothing if the
+  /// replacement is nullptr.
+  /// Assumes the list contains at most one Location decoration.
+  /// @param decos the decoration list to modify
+  /// @param replacement the location decoration to place into the list
+  /// @returns the location decoration that was replaced, if one was replaced,
+  /// or null otherwise.
+  ast::Decoration* SetLocation(ast::DecorationList* decos,
+                               ast::Decoration* replacement);
+
   /// Converts a SPIR-V struct member decoration. If the decoration is
   /// recognized but deliberately dropped, then returns nullptr without a
   /// diagnostic. On failure, emits a diagnostic and returns nullptr.
@@ -393,18 +404,12 @@ class ParserImpl : Reader {
   /// @returns the field name
   std::string GetMemberName(const Struct& struct_type, int member_index);
 
-  /// Returns the location decoration, if any on a struct member.
+  /// Returns the SPIR-V decorations for pipeline IO, if any, on a struct
-  /// @param struct_type the parser's structure type.
+  /// member.
-  /// @param member_index the member index
-  /// @returns a newly created location node, or nullptr
-  ast::Decoration* GetMemberLocation(const Struct& struct_type,
-                                     int member_index);
-
-  /// Returns the SPIR-V interpolation decorations, if any, on a struct member.
   /// @param struct_type the parser's structure type.
   /// @param member_index the member index
   /// @returns a list of SPIR-V decorations.
-  DecorationList GetMemberInterpolationDecorations(const Struct& struct_type,
+  DecorationList GetMemberPipelineDecorations(const Struct& struct_type,
                                               int member_index);
 
   /// Creates an AST Variable node for a SPIR-V ID, including any attached