From 3f10421ceecdd931a8f7e4916ba799db27963405 Mon Sep 17 00:00:00 2001
From: dan sinclair <dsinclair@google.com>
Date: Thu, 16 Jul 2020 14:35:44 +0000
Subject: [PATCH] [msl-writer] Fixup matrix and array constructors.

This CL fixes up the array constructors to emit `{}` instead of
`<type>[<size>]()`. The matrix example is also updated to have the
correct data format for WGSL matrices which fixes the MSL output.

Bug: tint:8
Change-Id: I3a08a8814d4b8b38a57b6324e2182a271c958ef3
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/25060
Reviewed-by: David Neto <dneto@google.com>
---
 src/writer/msl/generator_impl.cc              | 17 +++++++----
 .../msl/generator_impl_constructor_test.cc    | 30 ++++++++++++-------
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/src/writer/msl/generator_impl.cc b/src/writer/msl/generator_impl.cc
index c8ec198d1e..c68d12eda8 100644
--- a/src/writer/msl/generator_impl.cc
+++ b/src/writer/msl/generator_impl.cc
@@ -543,12 +543,15 @@ bool GeneratorImpl::EmitContinue(ast::ContinueStatement*) {
 }
 
 bool GeneratorImpl::EmitTypeConstructor(ast::TypeConstructorExpression* expr) {
-  if (!EmitType(expr->type(), "")) {
-    return false;
+  if (expr->type()->IsArray()) {
+    out_ << "{";
+  } else {
+    if (!EmitType(expr->type(), "")) {
+      return false;
+    }
+    out_ << "(";
   }
 
-  out_ << "(";
-
   // If the type constructor is empty then we need to construct with the zero
   // value for all components.
   if (expr->values().empty()) {
@@ -569,7 +572,11 @@ bool GeneratorImpl::EmitTypeConstructor(ast::TypeConstructorExpression* expr) {
     }
   }
 
-  out_ << ")";
+  if (expr->type()->IsArray()) {
+    out_ << "}";
+  } else {
+    out_ << ")";
+  }
   return true;
 }
 
diff --git a/src/writer/msl/generator_impl_constructor_test.cc b/src/writer/msl/generator_impl_constructor_test.cc
index ba2518d998..281a1a00b9 100644
--- a/src/writer/msl/generator_impl_constructor_test.cc
+++ b/src/writer/msl/generator_impl_constructor_test.cc
@@ -171,23 +171,29 @@ TEST_F(MslGeneratorImplTest, EmitConstructor_Type_Vec_Empty) {
 
 TEST_F(MslGeneratorImplTest, EmitConstructor_Type_Mat) {
   ast::type::F32Type f32;
-  ast::type::MatrixType mat(&f32, 3, 2);
+  ast::type::MatrixType mat(&f32, 3, 2);  // 3 ROWS, 2 COLUMNS
+  ast::type::VectorType vec(&f32, 3);
 
-  ast::type::VectorType vec(&f32, 2);
+  // WGSL matrix is mat2x3 (it flips for AST, sigh). With a type constructor
+  // of <vec3, vec3>
 
   ast::ExpressionList mat_values;
 
-  for (size_t i = 0; i < 3; i++) {
+  for (size_t i = 0; i < 2; i++) {
     auto lit1 = std::make_unique<ast::FloatLiteral>(
         &f32, static_cast<float>(1 + (i * 2)));
     auto lit2 = std::make_unique<ast::FloatLiteral>(
         &f32, static_cast<float>(2 + (i * 2)));
+    auto lit3 = std::make_unique<ast::FloatLiteral>(
+        &f32, static_cast<float>(3 + (i * 2)));
 
     ast::ExpressionList values;
     values.push_back(
         std::make_unique<ast::ScalarConstructorExpression>(std::move(lit1)));
     values.push_back(
         std::make_unique<ast::ScalarConstructorExpression>(std::move(lit2)));
+    values.push_back(
+        std::make_unique<ast::ScalarConstructorExpression>(std::move(lit3)));
 
     mat_values.push_back(std::make_unique<ast::TypeConstructorExpression>(
         &vec, std::move(values)));
@@ -197,14 +203,16 @@ TEST_F(MslGeneratorImplTest, EmitConstructor_Type_Mat) {
 
   GeneratorImpl g;
   ASSERT_TRUE(g.EmitConstructor(&expr)) << g.error();
-  EXPECT_EQ(g.result(),
-            std::string("float2x3(float2(1.00000000f, 2.00000000f), ") +
-                "float2(3.00000000f, 4.00000000f), " +
-                "float2(5.00000000f, 6.00000000f))");
+
+  // A matrix of type T with n columns and m rows can also be constructed from
+  // n vectors of type T with m components.
+  EXPECT_EQ(
+      g.result(),
+      std::string("float2x3(float3(1.00000000f, 2.00000000f, 3.00000000f), ") +
+          "float3(3.00000000f, 4.00000000f, 5.00000000f))");
 }
 
-// TODO(dsinclair): Verify
-TEST_F(MslGeneratorImplTest, DISABLED_EmitConstructor_Type_Array) {
+TEST_F(MslGeneratorImplTest, EmitConstructor_Type_Array) {
   ast::type::F32Type f32;
   ast::type::VectorType vec(&f32, 3);
   ast::type::ArrayType ary(&vec, 3);
@@ -235,10 +243,10 @@ TEST_F(MslGeneratorImplTest, DISABLED_EmitConstructor_Type_Array) {
 
   GeneratorImpl g;
   ASSERT_TRUE(g.EmitConstructor(&expr)) << g.error();
-  EXPECT_EQ(g.result(), std::string("float3[3](") +
+  EXPECT_EQ(g.result(), std::string("{") +
                             "float3(1.00000000f, 2.00000000f, 3.00000000f), " +
                             "float3(4.00000000f, 5.00000000f, 6.00000000f), " +
-                            "float3(7.00000000f, 8.00000000f, 9.00000000f))");
+                            "float3(7.00000000f, 8.00000000f, 9.00000000f)}");
 }
 
 // TODO(dsinclair): Add struct constructor test.