spirv-reader: remove builtin-inputs special cases

Remove special case handling of pointers and values related to builtins SampleId, VertexIndex, and InstanceIndex. These map to private variables with store type matching the type stated in the SPIR-V code. There is no need to generate special case code for user-written functions accessing those variables. Therefore: - Remove SkipReason enums associated with those builtin inputs - Remove newly unreachable code. Bug: tint:508 Change-Id: I22ea86d49e14f171a92863d9f02145606ad37683 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/55321 Kokoro: Kokoro <noreply+kokoro@google.com> Reviewed-by: James Price <jrprice@google.com> Commit-Queue: James Price <jrprice@google.com> Auto-Submit: David Neto <dneto@google.com>
2021-06-21 19:21:36 +00:00 · 2021-06-21 19:21:36 +00:00 · 3d5453ce9c
parent 4d94eee072
commit 3d5453ce9c
3 changed files with 147 additions and 151 deletions
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@ -2309,20 +2309,6 @@ TypedExpression FunctionEmitter::MakeExpression(uint32_t id) {
      Fail() << "unhandled use of a pointer to the PointSize builtin, with ID: "
             << id;
      return {};
-    case SkipReason::kSampleIdBuiltinPointer:
-      Fail() << "unhandled use of a pointer to the SampleId builtin, with ID: "
-             << id;
-      return {};
-    case SkipReason::kVertexIndexBuiltinPointer:
-      Fail()
-          << "unhandled use of a pointer to the VertexIndex builtin, with ID: "
-          << id;
-      return {};
-    case SkipReason::kInstanceIndexBuiltinPointer:
-      Fail() << "unhandled use of a pointer to the InstanceIndex builtin, with "
-                "ID: "
-             << id;
-      return {};
    case SkipReason::kSampleMaskInBuiltinPointer:
      Fail()
          << "unhandled use of a pointer to the SampleMask builtin, with ID: "
@ -3423,22 +3409,6 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) {
          GetDefInfo(inst.result_id())->skip =
              SkipReason::kPointSizeBuiltinValue;
          return true;
-        case SkipReason::kSampleIdBuiltinPointer:
-        case SkipReason::kVertexIndexBuiltinPointer:
-        case SkipReason::kInstanceIndexBuiltinPointer: {
-          auto name = NameForSpecialInputBuiltin(skip_reason);
-          if (name.empty()) {
-            return Fail() << "internal error: unhandled special input builtin "
-                             "variable: "
-                          << inst.PrettyPrint();
-          }
-          ast::Expression* id_expr = create<ast::IdentifierExpression>(
-              Source{}, builder_.Symbols().Register(name));
-
-          auto* loaded_type = parser_impl_.ConvertType(inst.type_id());
-          auto expr = TypedExpression{loaded_type, id_expr};
-          return EmitConstDefinition(inst, expr);
-        }
        case SkipReason::kSampleMaskInBuiltinPointer: {
          auto name = namer_.Name(sample_mask_in_id);
          ast::Expression* id_expr = create<ast::IdentifierExpression>(
@ -3573,28 +3543,6 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) {
                << inst.PrettyPrint();
 }

-std::string FunctionEmitter::NameForSpecialInputBuiltin(
-    SkipReason skip_reason) {
-  SpvBuiltIn spv_builtin = SpvBuiltIn(0);
-  switch (skip_reason) {
-    case SkipReason::kSampleIdBuiltinPointer:
-      spv_builtin = SpvBuiltInSampleId;
-      break;
-    case SkipReason::kVertexIndexBuiltinPointer:
-      spv_builtin = SpvBuiltInVertexIndex;
-      break;
-    case SkipReason::kInstanceIndexBuiltinPointer:
-      spv_builtin = SpvBuiltInInstanceIndex;
-      break;
-    default:
-      // Invalid. Issue the error in the caller.
-      return "";
-  }
-  // The SPIR-V variable is i32, but WGSL requires u32.
-  auto var_id = parser_impl_.IdForSpecialBuiltIn(spv_builtin);
-  return namer_.Name(var_id);
-}
-
 TypedExpression FunctionEmitter::MakeOperand(
    const spvtools::opt::Instruction& inst,
    uint32_t operand_index) {
@ -4279,15 +4227,6 @@ bool FunctionEmitter::RegisterSpecialBuiltInVariables() {
      case SpvBuiltInPointSize:
        def->skip = SkipReason::kPointSizeBuiltinPointer;
        break;
-      case SpvBuiltInSampleId:
-        def->skip = SkipReason::kSampleIdBuiltinPointer;
-        break;
-      case SpvBuiltInVertexIndex:
-        def->skip = SkipReason::kVertexIndexBuiltinPointer;
-        break;
-      case SpvBuiltInInstanceIndex:
-        def->skip = SkipReason::kInstanceIndexBuiltinPointer;
-        break;
      case SpvBuiltInSampleMask: {
        // Distinguish between input and output variable.
        const auto storage_class =
@ -4301,6 +4240,9 @@ bool FunctionEmitter::RegisterSpecialBuiltInVariables() {
        }
        break;
      }
+      case SpvBuiltInSampleId:
+      case SpvBuiltInInstanceIndex:
+      case SpvBuiltInVertexIndex:
      case SpvBuiltInLocalInvocationIndex:
      case SpvBuiltInLocalInvocationId:
      case SpvBuiltInGlobalInvocationId:
--- a/src/reader/spirv/function.h
+++ b/src/reader/spirv/function.h
@ -214,19 +214,6 @@ enum class SkipReason {
  /// supported by WebGPU.
  kPointSizeBuiltinValue,

-  /// `kSampleIdBuiltinPointer`: the value is a pointer to the SampleId builtin
-  /// variable.  Don't generate its address.
-  kSampleIdBuiltinPointer,
-
-  /// `kVertexIndexBuiltinPointer`: the value is a pointer to the VertexIndex
-  /// builtin variable.  Don't generate its address.
-  kVertexIndexBuiltinPointer,
-
-  /// `kInstanceIndexBuiltinPointer`: the value is a pointer to the
-  /// InstanceIndex
-  /// builtin variable.  Don't generate its address.
-  kInstanceIndexBuiltinPointer,
-
  /// `kSampleMaskInBuiltinPointer`: the value is a pointer to the SampleMaskIn
  /// builtin input variable.  Don't generate its address.
  kSampleMaskInBuiltinPointer,
@ -339,15 +326,6 @@ inline std::ostream& operator<<(std::ostream& o, const DefInfo& di) {
    case SkipReason::kPointSizeBuiltinValue:
      o << " skip:pointsize_value";
      break;
-    case SkipReason::kSampleIdBuiltinPointer:
-      o << " skip:sampleid_pointer";
-      break;
-    case SkipReason::kVertexIndexBuiltinPointer:
-      o << " skip:vertexindex_pointer";
-      break;
-    case SkipReason::kInstanceIndexBuiltinPointer:
-      o << " skip:instanceindex_pointer";
-      break;
    case SkipReason::kSampleMaskInBuiltinPointer:
      o << " skip:samplemaskin_pointer";
      break;
@ -795,13 +773,6 @@ class FunctionEmitter {
    return SkipReason::kDontSkip;
  }

-  /// Returns the WGSL variable name for an input builtin variable whose
-  /// translation is managed via the SkipReason mechanism.
-  /// @param skip_reason the skip reason for the special variable
-  /// @returns the variable name for a special builtin variable
-  /// that is handled via the "skip" mechanism.
-  std::string NameForSpecialInputBuiltin(SkipReason skip_reason);
-
  /// Returns the most deeply nested structured construct which encloses the
  /// WGSL scopes of names declared in both block positions. Each position must
  /// be a valid index into the function block order array.
--- a/src/reader/spirv/parser_impl_module_var_test.cc
+++ b/src/reader/spirv/parser_impl_module_var_test.cc
@ -2443,18 +2443,31 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_I32_Load_CopyObject) {
  ASSERT_TRUE(p->BuildAndParseInternalModule()) << p->error() << assembly;
  EXPECT_TRUE(p->error().empty());
  const auto module_str = p->program().to_str();
-
-  // Correct declaration
-  EXPECT_THAT(module_str, HasSubstr(R"(
+  const std::string expected =
+      R"(Module{
  Variable{
    x_1
    private
    undefined
    __i32
-  })")) <<module_str;
-
-  // Correct creation of value
-  EXPECT_THAT(module_str, HasSubstr(R"(
+  }
+  Function main_1 -> __void
+  ()
+  {
+    VariableDeclStatement{
+      VariableConst{
+        x_11
+        none
+        undefined
+        __ptr_none__i32
+        {
+          UnaryOp[not set]{
+            address-of
+            Identifier[not set]{x_1}
+          }
+        }
+      }
+    }
    VariableDeclStatement{
      VariableConst{
        x_2
@ -2462,14 +2475,15 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_I32_Load_CopyObject) {
        undefined
        __i32
        {
-          Identifier[not set]{x_1}
+          UnaryOp[not set]{
+            indirection
+            Identifier[not set]{x_11}
+          }
        }
      }
-    })"))
-      << module_str;
-
-  // Correct parameter on entry point
-  EXPECT_THAT(module_str, HasSubstr(R"(
+    }
+    Return{}
+  }
  Function main -> __void
  StageDecoration{fragment}
  (
@ -2489,8 +2503,15 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_I32_Load_CopyObject) {
      Bitcast[not set]<__i32>{
        Identifier[not set]{x_1_param}
      }
-    })"))
-      << module_str;
+    }
+    Call[not set]{
+      Identifier[not set]{main_1}
+      (
+      )
+    }
+  }
+}
+)";
 }

 TEST_F(SpvModuleScopeVarParserTest, SampleId_I32_Load_AccessChain) {
@ -2668,6 +2689,20 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_U32_Load_CopyObject) {
  Function main_1 -> __void
  ()
  {
+    VariableDeclStatement{
+      VariableConst{
+        x_11
+        none
+        undefined
+        __ptr_none__u32
+        {
+          UnaryOp[not set]{
+            address-of
+            Identifier[not set]{x_1}
+          }
+        }
+      }
+    }
    VariableDeclStatement{
      VariableConst{
        x_2
@ -2675,7 +2710,10 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_U32_Load_CopyObject) {
        undefined
        __u32
        {
-          Identifier[not set]{x_1}
+          UnaryOp[not set]{
+            indirection
+            Identifier[not set]{x_11}
+          }
        }
      }
    }
@ -2944,18 +2982,16 @@ TEST_F(SpvModuleScopeVarParserTest, SampleMask_In_U32_CopyObject) {
  ASSERT_TRUE(p->BuildAndParseInternalModule()) << p->error() << assembly;
  EXPECT_TRUE(p->error().empty());
  const auto module_str = p->program().to_str();
-
-  // Correct declaration
-  EXPECT_THAT(module_str, HasSubstr(R"(
+  const std::string expected = R"(Module{
  Variable{
    x_1
    private
    undefined
    __array__u32_1
-  })")) <<module_str;
-
-  // Correct creation of value
-  EXPECT_THAT(module_str, HasSubstr(R"(
+  }
+  Function main_1 -> __void
+  ()
+  {
    VariableDeclStatement{
      VariableConst{
        x_4
@ -2969,10 +3005,9 @@ TEST_F(SpvModuleScopeVarParserTest, SampleMask_In_U32_CopyObject) {
          }
        }
      }
-    })"));
-
-  // Correct parameter on entry point
-  EXPECT_THAT(module_str, HasSubstr(R"(
+    }
+    Return{}
+  }
  Function main -> __void
  StageDecoration{fragment}
  (
@ -2993,8 +3028,16 @@ TEST_F(SpvModuleScopeVarParserTest, SampleMask_In_U32_CopyObject) {
        ScalarConstructor[not set]{0}
      }
      Identifier[not set]{x_1_param}
-    })"))
-      << module_str;
+    }
+    Call[not set]{
+      Identifier[not set]{main_1}
+      (
+      )
+    }
+  }
+}
+)";
+  EXPECT_EQ(module_str, expected) << module_str;
 }

 TEST_F(SpvModuleScopeVarParserTest, SampleMask_In_U32_AccessChain) {
@ -3955,6 +3998,20 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_Load_CopyObject) {
  Function main_1 -> __void
  ()
  {
+    VariableDeclStatement{
+      VariableConst{
+        x_11
+        none
+        undefined
+        __ptr_none__i32
+        {
+          UnaryOp[not set]{
+            address-of
+            Identifier[not set]{x_1}
+          }
+        }
+      }
+    }
    VariableDeclStatement{
      VariableConst{
        x_2
@ -3962,7 +4019,10 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_Load_CopyObject) {
        undefined
        __i32
        {
-          Identifier[not set]{x_1}
+          UnaryOp[not set]{
+            indirection
+            Identifier[not set]{x_11}
+          }
        }
      }
    }
@ -4066,34 +4126,6 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_Load_AccessChain) {
  EXPECT_EQ(module_str, expected);
 }

-TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_FunctParam) {
-  // TODO(dneto): Passing by pointer-to-input is not allowed.
-  // Remove this test.
-  const std::string assembly = VertexIndexPreamble("%int") + R"(
-    %helper_ty = OpTypeFunction %int %ptr_ty
-    %helper = OpFunction %int None %helper_ty
-    %param = OpFunctionParameter %ptr_ty
-    %helper_entry = OpLabel
-    %3 = OpLoad %int %param
-    OpReturnValue %3
-    OpFunctionEnd
-
-    %main = OpFunction %void None %voidfn
-    %entry = OpLabel
-    %result = OpFunctionCall %int %helper %1
-    OpReturn
-    OpFunctionEnd
- )";
-  auto p = parser(test::Assemble(assembly));
-  // TODO(dneto): We can handle this if we make a shadow variable and mutate
-  // the parameter type.
-  ASSERT_FALSE(p->BuildAndParseInternalModule());
-  EXPECT_THAT(
-      p->error(),
-      HasSubstr(
-          "unhandled use of a pointer to the VertexIndex builtin, with ID: 1"));
-}
-
 TEST_F(SpvModuleScopeVarParserTest, VertexIndex_U32_Load_Direct) {
  const std::string assembly = VertexIndexPreamble("%uint") + R"(
    %main = OpFunction %void None %voidfn
@ -4181,6 +4213,20 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_U32_Load_CopyObject) {
  Function main_1 -> __void
  ()
  {
+    VariableDeclStatement{
+      VariableConst{
+        x_11
+        none
+        undefined
+        __ptr_none__u32
+        {
+          UnaryOp[not set]{
+            address-of
+            Identifier[not set]{x_1}
+          }
+        }
+      }
+    }
    VariableDeclStatement{
      VariableConst{
        x_2
@ -4188,7 +4234,10 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_U32_Load_CopyObject) {
        undefined
        __u32
        {
-          Identifier[not set]{x_1}
+          UnaryOp[not set]{
+            indirection
+            Identifier[not set]{x_11}
+          }
        }
      }
    }
@ -4421,6 +4470,20 @@ TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_I32_Load_CopyObject) {
  Function main_1 -> __void
  ()
  {
+    VariableDeclStatement{
+      VariableConst{
+        x_11
+        none
+        undefined
+        __ptr_none__i32
+        {
+          UnaryOp[not set]{
+            address-of
+            Identifier[not set]{x_1}
+          }
+        }
+      }
+    }
    VariableDeclStatement{
      VariableConst{
        x_2
@ -4428,7 +4491,10 @@ TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_I32_Load_CopyObject) {
        undefined
        __i32
        {
-          Identifier[not set]{x_1}
+          UnaryOp[not set]{
+            indirection
+            Identifier[not set]{x_11}
+          }
        }
      }
    }
@ -4643,6 +4709,20 @@ TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_U32_Load_CopyObject) {
  Function main_1 -> __void
  ()
  {
+    VariableDeclStatement{
+      VariableConst{
+        x_11
+        none
+        undefined
+        __ptr_none__u32
+        {
+          UnaryOp[not set]{
+            address-of
+            Identifier[not set]{x_1}
+          }
+        }
+      }
+    }
    VariableDeclStatement{
      VariableConst{
        x_2
@ -4650,7 +4730,10 @@ TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_U32_Load_CopyObject) {
        undefined
        __u32
        {
-          Identifier[not set]{x_1}
+          UnaryOp[not set]{
+            indirection
+            Identifier[not set]{x_11}
+          }
        }
      }
    }