writer/msl: Emit builtins as parameters

Add a config parameter for the CanonicalizeEntryPoint transform that selects between emitting builtins as parameters (for MSL) or struct members (for HLSL). This fixes all of the shader IO issues in Tint's E2E tests for MSL. Fixed: tint:817 Change-Id: Ieb31cdbd2e4d96ac41f8d8515fd07ead8241d770 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/53282 Kokoro: Kokoro <noreply+kokoro@google.com> Reviewed-by: Ben Clayton <bclayton@google.com> Commit-Queue: James Price <jrprice@google.com>
2025-12-13 23:26:24 +00:00 · 2021-06-04 14:40:28 +00:00
parent 3604e80321
commit 7697c31e84
21 changed files with 445 additions and 133 deletions
--- a/src/transform/canonicalize_entry_point_io.cc
+++ b/src/transform/canonicalize_entry_point_io.cc
@@ -24,6 +24,8 @@
 #include "src/sem/struct.h"
 #include "src/sem/variable.h"

+TINT_INSTANTIATE_TYPEINFO(tint::transform::CanonicalizeEntryPointIO::Config);
+
 namespace tint {
 namespace transform {

@@ -60,10 +62,17 @@ bool StructMemberComparator(const ast::StructMember* a,

 }  // namespace

-Output CanonicalizeEntryPointIO::Run(const Program* in, const DataMap&) {
+Output CanonicalizeEntryPointIO::Run(const Program* in, const DataMap& data) {
  ProgramBuilder out;
  CloneContext ctx(&out, in);

+  auto* cfg = data.Get<Config>();
+  if (cfg == nullptr) {
+    out.Diagnostics().add_error(
+        "missing transform data for CanonicalizeEntryPointIO");
+    return Output(Program(std::move(out)));
+  }
+
  // Strip entry point IO decorations from struct declarations.
  // TODO(jrprice): This code is duplicated with the SPIR-V transform.
  for (auto* ty : ctx.src->AST().ConstructedTypes()) {
@@ -104,6 +113,16 @@ Output CanonicalizeEntryPointIO::Run(const Program* in, const DataMap&) {
      auto new_struct_param_symbol = ctx.dst->Sym();
      ast::StructMemberList new_struct_members;
      for (auto* param : func->Parameters()) {
+        if (cfg->builtin_style == BuiltinStyle::kParameter &&
+            ast::HasDecoration<ast::BuiltinDecoration>(
+                param->Declaration()->decorations())) {
+          // If this parameter is a builtin and we are emitting those as
+          // parameters, then just clone it as is.
+          new_parameters.push_back(
+              ctx.Clone(const_cast<ast::Variable*>(param->Declaration())));
+          continue;
+        }
+
        auto param_name = ctx.Clone(param->Declaration()->symbol());
        auto* param_ty = param->Type();
        auto* param_declared_ty = param->Declaration()->type();
@@ -118,6 +137,20 @@ Output CanonicalizeEntryPointIO::Run(const Program* in, const DataMap&) {
              TINT_ICE(ctx.dst->Diagnostics()) << "nested pipeline IO struct";
            }

+            if (cfg->builtin_style == BuiltinStyle::kParameter &&
+                ast::HasDecoration<ast::BuiltinDecoration>(
+                    member->Declaration()->decorations())) {
+              // If this struct member is a builtin and we are emitting those as
+              // parameters, then move it to the parameter list.
+              auto* member_ty = CreateASTTypeFor(&ctx, member->Type());
+              auto new_param_name = ctx.dst->Sym();
+              new_parameters.push_back(ctx.dst->Param(
+                  new_param_name, member_ty,
+                  ctx.Clone(member->Declaration()->decorations())));
+              init_values.push_back(ctx.dst->Expr(new_param_name));
+              continue;
+            }
+
            ast::DecorationList new_decorations = RemoveDecorations(
                &ctx, member->Declaration()->decorations(),
                [](const ast::Decoration* deco) {
@@ -161,21 +194,23 @@ Output CanonicalizeEntryPointIO::Run(const Program* in, const DataMap&) {
        }
      }

-      // Sort struct members to satisfy HLSL interfacing matching rules.
-      std::sort(new_struct_members.begin(), new_struct_members.end(),
-                StructMemberComparator);
+      if (!new_struct_members.empty()) {
+        // Sort struct members to satisfy HLSL interfacing matching rules.
+        std::sort(new_struct_members.begin(), new_struct_members.end(),
+                  StructMemberComparator);

-      // Create the new struct type.
-      auto in_struct_name = ctx.dst->Sym();
-      auto* in_struct = ctx.dst->create<ast::Struct>(
-          in_struct_name, new_struct_members, ast::DecorationList{});
-      ctx.InsertBefore(ctx.src->AST().GlobalDeclarations(), func_ast,
-                       in_struct);
+        // Create the new struct type.
+        auto in_struct_name = ctx.dst->Sym();
+        auto* in_struct = ctx.dst->create<ast::Struct>(
+            in_struct_name, new_struct_members, ast::DecorationList{});
+        ctx.InsertBefore(ctx.src->AST().GlobalDeclarations(), func_ast,
+                         in_struct);

-      // Create a new function parameter using this struct type.
-      auto* struct_param = ctx.dst->Param(
-          new_struct_param_symbol, ctx.dst->ty.type_name(in_struct_name));
-      new_parameters.push_back(struct_param);
+        // Create a new function parameter using this struct type.
+        auto* struct_param = ctx.dst->Param(
+            new_struct_param_symbol, ctx.dst->ty.type_name(in_struct_name));
+        new_parameters.push_back(struct_param);
+      }
    }

    // Handle return type.
@@ -273,5 +308,13 @@ Output CanonicalizeEntryPointIO::Run(const Program* in, const DataMap&) {
  return Output(Program(std::move(out)));
 }

+CanonicalizeEntryPointIO::Config::Config(BuiltinStyle builtins)
+    : builtin_style(builtins) {}
+
+CanonicalizeEntryPointIO::Config::Config(const Config&) = default;
+CanonicalizeEntryPointIO::Config::~Config() = default;
+CanonicalizeEntryPointIO::Config& CanonicalizeEntryPointIO::Config::operator=(
+    const Config&) = default;
+
 }  // namespace transform
 }  // namespace tint
--- a/src/transform/canonicalize_entry_point_io.h
+++ b/src/transform/canonicalize_entry_point_io.h
@@ -70,6 +70,34 @@ namespace transform {
 /// ```
 class CanonicalizeEntryPointIO : public Transform {
 public:
+  /// BuiltinStyle is an enumerator of different ways to emit builtins.
+  enum class BuiltinStyle {
+    /// Use non-struct function parameters for all builtins.
+    kParameter,
+    /// Use struct members for all builtins.
+    kStructMember,
+  };
+
+  /// Configuration options for the transform.
+  struct Config : public Castable<Config, Data> {
+    /// Constructor
+    /// @param builtins the approach to use for emitting builtins.
+    explicit Config(BuiltinStyle builtins);
+
+    /// Copy constructor
+    Config(const Config&);
+
+    /// Destructor
+    ~Config() override;
+
+    /// Assignment operator
+    /// @returns this Config
+    Config& operator=(const Config&);
+
+    /// The approach to use for emitting builtins.
+    BuiltinStyle builtin_style;
+  };
+
  /// Constructor
  CanonicalizeEntryPointIO();
  ~CanonicalizeEntryPointIO() override;
--- a/src/transform/canonicalize_entry_point_io_test.cc
+++ b/src/transform/canonicalize_entry_point_io_test.cc
@@ -22,7 +22,51 @@ namespace {

 using CanonicalizeEntryPointIOTest = TransformTest;

-TEST_F(CanonicalizeEntryPointIOTest, Parameters) {
+TEST_F(CanonicalizeEntryPointIOTest, Error_MissingTransformData) {
+  auto* src = "";
+
+  auto* expect = "error: missing transform data for CanonicalizeEntryPointIO";
+
+  auto got = Run<CanonicalizeEntryPointIO>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(CanonicalizeEntryPointIOTest, Parameters_BuiltinsAsParameters) {
+  auto* src = R"(
+[[stage(fragment)]]
+fn frag_main([[location(1)]] loc1 : f32,
+             [[location(2)]] loc2 : vec4<u32>,
+             [[builtin(position)]] coord : vec4<f32>) {
+  var col : f32 = (coord.x * loc1);
+}
+)";
+
+  auto* expect = R"(
+struct tint_symbol_1 {
+  [[location(1)]]
+  loc1 : f32;
+  [[location(2)]]
+  loc2 : vec4<u32>;
+};
+
+[[stage(fragment)]]
+fn frag_main([[builtin(position)]] coord : vec4<f32>, tint_symbol : tint_symbol_1) {
+  let loc1 : f32 = tint_symbol.loc1;
+  let loc2 : vec4<u32> = tint_symbol.loc2;
+  var col : f32 = (coord.x * loc1);
+}
+)";
+
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(CanonicalizeEntryPointIOTest, Parameters_BuiltinsAsStructMembers) {
  auto* src = R"(
 [[stage(fragment)]]
 fn frag_main([[location(1)]] loc1 : f32,
@@ -51,7 +95,10 @@ fn frag_main(tint_symbol : tint_symbol_1) {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kStructMember);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
@@ -81,12 +128,49 @@ fn frag_main(tint_symbol : tint_symbol_1) {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }

-TEST_F(CanonicalizeEntryPointIOTest, Parameters_EmptyBody) {
+TEST_F(CanonicalizeEntryPointIOTest,
+       Parameters_EmptyBody_BuiltinsAsParameters) {
+  auto* src = R"(
+[[stage(fragment)]]
+fn frag_main([[location(1)]] loc1 : f32,
+             [[location(2)]] loc2 : vec4<u32>,
+             [[builtin(position)]] coord : vec4<f32>) {
+}
+)";
+
+  auto* expect = R"(
+struct tint_symbol_1 {
+  [[location(1)]]
+  loc1 : f32;
+  [[location(2)]]
+  loc2 : vec4<u32>;
+};
+
+[[stage(fragment)]]
+fn frag_main([[builtin(position)]] coord : vec4<f32>, tint_symbol : tint_symbol_1) {
+  let loc1 : f32 = tint_symbol.loc1;
+  let loc2 : vec4<u32> = tint_symbol.loc2;
+}
+)";
+
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(CanonicalizeEntryPointIOTest,
+       Parameters_EmptyBody_BuiltinsAsStructMembers) {
  auto* src = R"(
 [[stage(fragment)]]
 fn frag_main([[location(1)]] loc1 : f32,
@@ -113,12 +197,69 @@ fn frag_main(tint_symbol : tint_symbol_1) {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kStructMember);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }

-TEST_F(CanonicalizeEntryPointIOTest, StructParameters) {
+TEST_F(CanonicalizeEntryPointIOTest, StructParameters_BuiltinsAsParameters) {
+  auto* src = R"(
+struct FragBuiltins {
+  [[builtin(position)]] coord : vec4<f32>;
+};
+struct FragLocations {
+  [[location(1)]] loc1 : f32;
+  [[location(2)]] loc2 : vec4<u32>;
+};
+
+[[stage(fragment)]]
+fn frag_main([[location(0)]] loc0 : f32,
+             locations : FragLocations,
+             builtins : FragBuiltins) {
+  var col : f32 = ((builtins.coord.x * locations.loc1) + loc0);
+}
+)";
+
+  auto* expect = R"(
+struct FragBuiltins {
+  coord : vec4<f32>;
+};
+
+struct FragLocations {
+  loc1 : f32;
+  loc2 : vec4<u32>;
+};
+
+struct tint_symbol_2 {
+  [[location(0)]]
+  loc0 : f32;
+  [[location(1)]]
+  loc1 : f32;
+  [[location(2)]]
+  loc2 : vec4<u32>;
+};
+
+[[stage(fragment)]]
+fn frag_main([[builtin(position)]] tint_symbol_1 : vec4<f32>, tint_symbol : tint_symbol_2) {
+  let loc0 : f32 = tint_symbol.loc0;
+  let locations : FragLocations = FragLocations(tint_symbol.loc1, tint_symbol.loc2);
+  let builtins : FragBuiltins = FragBuiltins(tint_symbol_1);
+  var col : f32 = ((builtins.coord.x * locations.loc1) + loc0);
+}
+)";
+
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(CanonicalizeEntryPointIOTest, StructParameters_BuiltinsAsStructMembers) {
  auto* src = R"(
 struct FragBuiltins {
  [[builtin(position)]] coord : vec4<f32>;
@@ -166,7 +307,10 @@ fn frag_main(tint_symbol : tint_symbol_1) {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kStructMember);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
@@ -191,7 +335,10 @@ fn frag_main() -> tint_symbol {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
@@ -240,7 +387,10 @@ fn frag_main() -> tint_symbol {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
@@ -304,7 +454,10 @@ fn frag_main2(tint_symbol_2 : tint_symbol_3) {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
@@ -366,7 +519,10 @@ fn frag_main1(tint_symbol : tint_symbol_1) {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
@@ -444,7 +600,10 @@ fn frag_main(tint_symbol : tint_symbol_1) -> tint_symbol_2 {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
@@ -501,7 +660,10 @@ fn frag_main(tint_symbol : tint_symbol_1) -> tint_symbol_2 {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kStructMember);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
@@ -593,7 +755,10 @@ fn frag_main(tint_symbol_2 : tint_symbol_3) {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kStructMember);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
@@ -617,7 +782,10 @@ fn tint_symbol_1(tint_symbol : tint_symbol_2) {
 }
 )";

-  auto got = Run<CanonicalizeEntryPointIO>(src);
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);

  EXPECT_EQ(expect, str(got));
 }
--- a/src/transform/hlsl.cc
+++ b/src/transform/hlsl.cc
@@ -32,8 +32,9 @@ namespace transform {
 Hlsl::Hlsl() = default;
 Hlsl::~Hlsl() = default;

-Output Hlsl::Run(const Program* in, const DataMap& data) {
+Output Hlsl::Run(const Program* in, const DataMap&) {
  Manager manager;
+  DataMap data;
  manager.Add<CanonicalizeEntryPointIO>();
  manager.Add<InlinePointerLets>();
  // Simplify cleans up messy `*(&(expr))` expressions from InlinePointerLets.
@@ -46,6 +47,8 @@ Output Hlsl::Run(const Program* in, const DataMap& data) {
  manager.Add<CalculateArrayLength>();
  manager.Add<ExternalTextureTransform>();
  manager.Add<PromoteInitializersToConstVar>();
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kStructMember);
  auto out = manager.Run(in, data);
  if (!out.program.IsValid()) {
    return out;
--- a/src/transform/msl.cc
+++ b/src/transform/msl.cc
@@ -35,11 +35,14 @@ namespace transform {
 Msl::Msl() = default;
 Msl::~Msl() = default;

-Output Msl::Run(const Program* in, const DataMap& data) {
+Output Msl::Run(const Program* in, const DataMap&) {
  Manager manager;
+  DataMap data;
  manager.Add<CanonicalizeEntryPointIO>();
  manager.Add<ExternalTextureTransform>();
  manager.Add<PromoteInitializersToConstVar>();
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::BuiltinStyle::kParameter);
  auto out = manager.Run(in, data);
  if (!out.program.IsValid()) {
    return out;