From 5102f87e38e0092828664b0208e5398781e8686e Mon Sep 17 00:00:00 2001
From: James Price <jrprice@google.com>
Date: Thu, 27 May 2021 14:15:47 +0000
Subject: [PATCH] writer/msl: Add parentheses for member accesses

The LHS should be wrapped in parentheses if it has lower precedence
than the access. This fixes issues with pointer dereferences followed
by member accesses, where we were previously generating *a.b.

Fixed: tint:831
Change-Id: I8a194ad4f54c80a01c24eb983ec8064037575216
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/51963
Kokoro: Kokoro <noreply+kokoro@google.com>
Auto-Submit: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
---
 src/writer/msl/generator_impl.cc                | 11 +++++++++++
 test/bug/tint/749.spvasm.expected.msl           |  2 +-
 .../global/struct_field.spvasm.expected.msl     | 17 ++++++++++++++++-
 .../load/global/struct_field.wgsl.expected.msl  | 15 ++++++++++++++-
 .../global/struct_field.spvasm.expected.msl     | 15 ++++++++++++++-
 5 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/src/writer/msl/generator_impl.cc b/src/writer/msl/generator_impl.cc
index 11ee9a1a14..fd76e6ea66 100644
--- a/src/writer/msl/generator_impl.cc
+++ b/src/writer/msl/generator_impl.cc
@@ -1728,9 +1728,20 @@ bool GeneratorImpl::EmitIf(ast::IfStatement* stmt) {
 }
 
 bool GeneratorImpl::EmitMemberAccessor(ast::MemberAccessorExpression* expr) {
+  bool paren_lhs =
+      !expr->structure()
+           ->IsAnyOf<ast::ArrayAccessorExpression, ast::CallExpression,
+                     ast::IdentifierExpression, ast::MemberAccessorExpression,
+                     ast::TypeConstructorExpression>();
+  if (paren_lhs) {
+    out_ << "(";
+  }
   if (!EmitExpression(expr->structure())) {
     return false;
   }
+  if (paren_lhs) {
+    out_ << ")";
+  }
 
   out_ << ".";
 
diff --git a/test/bug/tint/749.spvasm.expected.msl b/test/bug/tint/749.spvasm.expected.msl
index 5a7c08c2b5..6bd339d73b 100644
--- a/test/bug/tint/749.spvasm.expected.msl
+++ b/test/bug/tint/749.spvasm.expected.msl
@@ -1 +1 @@
-SKIP: crbug.com/tint/831
+SKIP: crbug.com/tint/833 loop emission is broken
diff --git a/test/ptr_ref/load/global/struct_field.spvasm.expected.msl b/test/ptr_ref/load/global/struct_field.spvasm.expected.msl
index 5a7c08c2b5..e3352ce09c 100644
--- a/test/ptr_ref/load/global/struct_field.spvasm.expected.msl
+++ b/test/ptr_ref/load/global/struct_field.spvasm.expected.msl
@@ -1 +1,16 @@
-SKIP: crbug.com/tint/831
+#include <metal_stdlib>
+
+using namespace metal;
+struct S {
+  int i;
+};
+
+kernel void tint_symbol() {
+  thread S tint_symbol_2 = {};
+  thread S* const tint_symbol_1 = &(tint_symbol_2);
+  int i = 0;
+  int const x_15 = (*(tint_symbol_1)).i;
+  i = x_15;
+  return;
+}
+
diff --git a/test/ptr_ref/load/global/struct_field.wgsl.expected.msl b/test/ptr_ref/load/global/struct_field.wgsl.expected.msl
index 5a7c08c2b5..cca8c398b9 100644
--- a/test/ptr_ref/load/global/struct_field.wgsl.expected.msl
+++ b/test/ptr_ref/load/global/struct_field.wgsl.expected.msl
@@ -1 +1,14 @@
-SKIP: crbug.com/tint/831
+#include <metal_stdlib>
+
+using namespace metal;
+struct S {
+  int i;
+};
+
+kernel void tint_symbol() {
+  thread S tint_symbol_2 = {};
+  thread S* const tint_symbol_1 = &(tint_symbol_2);
+  int const i = (*(tint_symbol_1)).i;
+  return;
+}
+
diff --git a/test/ptr_ref/store/global/struct_field.spvasm.expected.msl b/test/ptr_ref/store/global/struct_field.spvasm.expected.msl
index 3bde28121e..f66e91934f 100644
--- a/test/ptr_ref/store/global/struct_field.spvasm.expected.msl
+++ b/test/ptr_ref/store/global/struct_field.spvasm.expected.msl
@@ -1 +1,14 @@
-SKIP: crbug.com/tint/831
\ No newline at end of file
+#include <metal_stdlib>
+
+using namespace metal;
+struct S {
+  int i;
+};
+
+kernel void tint_symbol() {
+  thread S tint_symbol_2 = {};
+  thread S* const tint_symbol_1 = &(tint_symbol_2);
+  (*(tint_symbol_1)).i = 5;
+  return;
+}
+