[spirv-writer] Fix accessing array of non-scalars.

Currently, if we access an array of non-scalar items we'll incorrectly
emit an OpVectorExtractDynamic which will fail as the result is not
scalar.

This CL updates the array accessor code such that if the base array is
an array of non-scalars we'll do load of the array and then access chain
into the loaded variable.

Bug: tint:267
Change-Id: Ia4d7052b57d8b31b835714b7b922c7859e3dce1f
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/29844
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Commit-Queue: David Neto <dneto@google.com>
Reviewed-by: David Neto <dneto@google.com>

src/type_determiner.cc

@@ -352,6 +352,13 @@ bool TypeDeterminer::DetermineArrayAccessor(
   if (res->IsPointer()) {
     ret = ctx_.type_mgr().Get(std::make_unique<ast::type::PointerType>(
         ret, res->AsPointer()->storage_class()));
+  } else if (parent_type->IsArray() &&
+             !parent_type->AsArray()->type()->is_scalar()) {
+    // If we extract a non-scalar from an array then we also get a pointer. We
+    // will generate a Function storage class variable to store this
+    // into.
+    ret = ctx_.type_mgr().Get(std::make_unique<ast::type::PointerType>(
+        ret, ast::StorageClass::kFunction));
   }
   expr->set_result_type(ret);
 

src/type_determiner_test.cc

@@ -528,7 +528,7 @@ TEST_F(TypeDeterminerTest, Expr_ArrayAccessor_Array_Constant) {
       std::make_unique<ast::IdentifierExpression>("my_var"), std::move(idx));
   EXPECT_TRUE(td()->DetermineResultType(&acc));
   ASSERT_NE(acc.result_type(), nullptr);
-  EXPECT_TRUE(acc.result_type()->IsF32());
+  EXPECT_TRUE(acc.result_type()->IsF32()) << acc.result_type()->type_name();
 }
 
 TEST_F(TypeDeterminerTest, Expr_ArrayAccessor_Matrix) {

src/writer/spirv/builder.cc

@@ -783,8 +783,11 @@ bool Builder::GenerateArrayAccessor(ast::ArrayAccessorExpression* expr,
   }
   idx_id = GenerateLoadIfNeeded(expr->idx_expr()->result_type(), idx_id);
 
-  // If the source is a pointer we access chain into it.
+  // If the source is a pointer we access chain into it. We also access chain
-  if (info->source_type->IsPointer()) {
+  // into an array of non-scalar types.
+  if (info->source_type->IsPointer() ||
+      (info->source_type->IsArray() &&
+       !info->source_type->AsArray()->type()->is_scalar())) {
     info->access_chain_indices.push_back(idx_id);
     info->source_type = expr->result_type();
     return true;
@@ -967,6 +970,37 @@ uint32_t Builder::GenerateAccessorExpression(ast::Expression* expr) {
   }
   info.source_type = source->result_type();
 
+  // If our initial access in into an array, and that array is not a pointer,
+  // then we need to load that array into a variable in order to be access
+  // chain into the array
+  if (accessors[0]->IsArrayAccessor()) {
+    auto* ary_res_type =
+        accessors[0]->AsArrayAccessor()->array()->result_type();
+    if (!ary_res_type->IsPointer()) {
+      ast::type::PointerType ptr(ary_res_type, ast::StorageClass::kFunction);
+      auto result_type_id = GenerateTypeIfNeeded(&ptr);
+      if (result_type_id == 0) {
+        return 0;
+      }
+
+      auto ary_result = result_op();
+
+      ast::NullLiteral nl(ary_res_type);
+      auto init = GenerateLiteralIfNeeded(nullptr, &nl);
+
+      // If we're access chaining into an array then we must be in a function
+      push_function_var(
+          {Operand::Int(result_type_id), ary_result,
+           Operand::Int(ConvertStorageClass(ast::StorageClass::kFunction)),
+           Operand::Int(init)});
+
+      push_function_inst(spv::Op::OpStore,
+                         {ary_result, Operand::Int(info.source_id)});
+
+      info.source_id = ary_result.to_i();
+    }
+  }
+
   std::vector<uint32_t> access_chain_indices;
   for (auto* accessor : accessors) {
     if (accessor->IsArrayAccessor()) {

src/writer/spirv/builder_accessor_expression_test.cc

@@ -28,7 +28,10 @@
 #include "src/ast/type/f32_type.h"
 #include "src/ast/type/i32_type.h"
 #include "src/ast/type/struct_type.h"
+#include "src/ast/type/u32_type.h"
 #include "src/ast/type/vector_type.h"
+#include "src/ast/type_constructor_expression.h"
+#include "src/ast/uint_literal.h"
 #include "src/ast/variable.h"
 #include "src/context.h"
 #include "src/type_determiner.h"
@@ -941,6 +944,90 @@ TEST_F(BuilderTest, Accessor_Mixed_ArrayAndMember) {
 )");
 }
 
+TEST_F(BuilderTest, Accessor_Array_Of_Vec) {
+  // const pos : array<vec2<f32>, 3> = array<vec2<f32>, 3>(
+  //   vec2<f32>(0.0, 0.5),
+  //   vec2<f32>(-0.5, -0.5),
+  //   vec2<f32>(0.5, -0.5));
+  // pos[1]
+
+  ast::type::F32Type f32;
+  ast::type::U32Type u32;
+  ast::type::VectorType vec(&f32, 2);
+  ast::type::ArrayType arr(&vec, 3);
+
+  ast::ExpressionList ary_params;
+
+  ast::ExpressionList vec_params;
+  vec_params.push_back(std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::FloatLiteral>(&f32, 0.0)));
+  vec_params.push_back(std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::FloatLiteral>(&f32, 0.5)));
+  ary_params.push_back(std::make_unique<ast::TypeConstructorExpression>(
+      &vec, std::move(vec_params)));
+
+  vec_params.push_back(std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::FloatLiteral>(&f32, -0.5)));
+  vec_params.push_back(std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::FloatLiteral>(&f32, -0.5)));
+  ary_params.push_back(std::make_unique<ast::TypeConstructorExpression>(
+      &vec, std::move(vec_params)));
+
+  vec_params.push_back(std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::FloatLiteral>(&f32, 0.5)));
+  vec_params.push_back(std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::FloatLiteral>(&f32, -0.5)));
+  ary_params.push_back(std::make_unique<ast::TypeConstructorExpression>(
+      &vec, std::move(vec_params)));
+
+  ast::Variable var("pos", ast::StorageClass::kPrivate, &arr);
+  var.set_is_const(true);
+  var.set_constructor(std::make_unique<ast::TypeConstructorExpression>(
+      &arr, std::move(ary_params)));
+
+  ast::ArrayAccessorExpression expr(
+      std::make_unique<ast::IdentifierExpression>("pos"),
+      std::make_unique<ast::ScalarConstructorExpression>(
+          std::make_unique<ast::UintLiteral>(&u32, 1)));
+
+  Context ctx;
+  ast::Module mod;
+  TypeDeterminer td(&ctx, &mod);
+  td.RegisterVariableForTesting(&var);
+  ASSERT_TRUE(td.DetermineResultType(var.constructor())) << td.error();
+  ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error();
+
+  Builder b(&mod);
+  b.push_function(Function{});
+  ASSERT_TRUE(b.GenerateFunctionVariable(&var)) << b.error();
+  EXPECT_EQ(b.GenerateAccessorExpression(&expr), 18u) << b.error();
+
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%3 = OpTypeFloat 32
+%2 = OpTypeVector %3 2
+%4 = OpTypeInt 32 0
+%5 = OpConstant %4 3
+%1 = OpTypeArray %2 %5
+%6 = OpConstant %3 0
+%7 = OpConstant %3 0.5
+%8 = OpConstantComposite %2 %6 %7
+%9 = OpConstant %3 -0.5
+%10 = OpConstantComposite %2 %9 %9
+%11 = OpConstantComposite %2 %7 %9
+%12 = OpConstantComposite %1 %8 %10 %11
+%13 = OpTypePointer Function %1
+%15 = OpConstantNull %1
+%16 = OpConstant %4 1
+%17 = OpTypePointer Function %2
+)");
+  EXPECT_EQ(DumpInstructions(b.functions()[0].variables()),
+            R"(%14 = OpVariable %13 Function %15
+)");
+  EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
+            R"(OpStore %14 %12
+%18 = OpAccessChain %17 %14 %16
+)");
+}
+
 TEST_F(BuilderTest, DISABLED_Accessor_Array_NonPointer) {
   // const a : array<f32, 3>;
   // a[2]