From 93e600dd54fc266dddb5cf05dab95a98e8b3ff40 Mon Sep 17 00:00:00 2001
From: Antonio Maiorano <amaiorano@google.com>
Date: Tue, 15 Mar 2022 13:51:17 +0000
Subject: [PATCH] Fix sem::Swizzle::HasSideEffects() not returning true when
 structure has side-effects

E.g. sem info for "f().x" returned false for HasSideEffects().

Bug: tint:1300
Change-Id: I789f75eef834c58a93e07d93c8334635d39981c3
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/83100
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
---
 src/tint/resolver/resolver.cc              | 10 +++++-----
 src/tint/resolver/side_effects_test.cc     | 14 +++++++++++++-
 src/tint/sem/member_accessor_expression.cc |  5 +++--
 src/tint/sem/member_accessor_expression.h  |  4 +++-
 4 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc
index d7c13a6a6d..5c7e36ae9e 100644
--- a/src/tint/resolver/resolver.cc
+++ b/src/tint/resolver/resolver.cc
@@ -1715,6 +1715,10 @@ sem::Expression* Resolver::MemberAccessor(
   const sem::Type* ret = nullptr;
   std::vector<uint32_t> swizzle;
 
+  // Structure may be a side-effecting expression (e.g. function call).
+  auto* sem_structure = Sem(expr->structure);
+  bool has_side_effects = sem_structure && sem_structure->HasSideEffects();
+
   if (auto* str = storage_ty->As<sem::Struct>()) {
     Mark(expr->member);
     auto symbol = expr->member->symbol;
@@ -1741,10 +1745,6 @@ sem::Expression* Resolver::MemberAccessor(
                                              ref->Access());
     }
 
-    // Structure may be a side-effecting expression (e.g. function call).
-    auto* sem_structure = Sem(expr->structure);
-    bool has_side_effects = sem_structure && sem_structure->HasSideEffects();
-
     return builder_->create<sem::StructMemberAccess>(
         expr, ret, current_statement_, member, has_side_effects);
   }
@@ -1819,7 +1819,7 @@ sem::Expression* Resolver::MemberAccessor(
                                           static_cast<uint32_t>(size));
     }
     return builder_->create<sem::Swizzle>(expr, ret, current_statement_,
-                                          std::move(swizzle));
+                                          std::move(swizzle), has_side_effects);
   }
 
   AddError(
diff --git a/src/tint/resolver/side_effects_test.cc b/src/tint/resolver/side_effects_test.cc
index 944ff5d18a..50f4e99d9d 100644
--- a/src/tint/resolver/side_effects_test.cc
+++ b/src/tint/resolver/side_effects_test.cc
@@ -217,7 +217,7 @@ TEST_F(SideEffectsTest, MemberAccessor_Vector) {
   EXPECT_FALSE(sem->HasSideEffects());
 }
 
-TEST_F(SideEffectsTest, MemberAccessor_VectorSwizzle) {
+TEST_F(SideEffectsTest, MemberAccessor_VectorSwizzleNoSE) {
   auto* var = Decl(Var("a", ty.vec4<f32>()));
   auto* expr = MemberAccessor("a", "xzyw");
   WrapInFunction(var, expr);
@@ -229,6 +229,18 @@ TEST_F(SideEffectsTest, MemberAccessor_VectorSwizzle) {
   EXPECT_FALSE(sem->HasSideEffects());
 }
 
+TEST_F(SideEffectsTest, MemberAccessor_VectorSwizzleSE) {
+  MakeSideEffectFunc("se", [&] { return ty.vec4<f32>(); });
+  auto* expr = MemberAccessor(Call("se"), "xzyw");
+  WrapInFunction(expr);
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+  auto* sem = Sem().Get(expr);
+  EXPECT_TRUE(sem->Is<sem::Swizzle>());
+  ASSERT_NE(sem, nullptr);
+  EXPECT_TRUE(sem->HasSideEffects());
+}
+
 TEST_F(SideEffectsTest, Binary_NoSE) {
   auto* a = Decl(Var("a", ty.i32()));
   auto* b = Decl(Var("b", ty.i32()));
diff --git a/src/tint/sem/member_accessor_expression.cc b/src/tint/sem/member_accessor_expression.cc
index 26f3842e33..53b1402c96 100644
--- a/src/tint/sem/member_accessor_expression.cc
+++ b/src/tint/sem/member_accessor_expression.cc
@@ -46,8 +46,9 @@ StructMemberAccess::~StructMemberAccess() = default;
 Swizzle::Swizzle(const ast::MemberAccessorExpression* declaration,
                  const sem::Type* type,
                  const Statement* statement,
-                 std::vector<uint32_t> indices)
-    : Base(declaration, type, statement, /* has_side_effects */ false),
+                 std::vector<uint32_t> indices,
+                 bool has_side_effects)
+    : Base(declaration, type, statement, has_side_effects),
       indices_(std::move(indices)) {}
 
 Swizzle::~Swizzle() = default;
diff --git a/src/tint/sem/member_accessor_expression.h b/src/tint/sem/member_accessor_expression.h
index df4950401a..9eed53b29f 100644
--- a/src/tint/sem/member_accessor_expression.h
+++ b/src/tint/sem/member_accessor_expression.h
@@ -88,10 +88,12 @@ class Swizzle final : public Castable<Swizzle, MemberAccessorExpression> {
   /// @param type the resolved type of the expression
   /// @param statement the statement that owns this expression
   /// @param indices the swizzle indices
+  /// @param has_side_effects whether this expression may have side effects
   Swizzle(const ast::MemberAccessorExpression* declaration,
           const sem::Type* type,
           const Statement* statement,
-          std::vector<uint32_t> indices);
+          std::vector<uint32_t> indices,
+          bool has_side_effects);
 
   /// Destructor
   ~Swizzle() override;