commit 93e600dd54fc266dddb5cf05dab95a98e8b3ff40
author Antonio Maiorano <amaiorano@google.com> Tue Mar 15 13:51:17 2022 +0000
committer Tint LUCI CQ <tint-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Mar 15 13:51:17 2022 +0000
tree 684931f36d2ae13504ff6c368a68ae3ececf01a9
parent 383e9b7b868b372d333588efedcf825f294864ff

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

diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc
index d7c13a6..5c7e36a 100644
--- a/src/tint/resolver/resolver.cc
+++ b/src/tint/resolver/resolver.cc
@@ -1715,6 +1715,10 @@
   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 @@
                                              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 @@
                                           static_cast<uint32_t>(size));
     }
     return builder_->create<sem::Swizzle>(expr, ret, current_statement_,
-                                          std::move(swizzle));
+                                          std::move(swizzle), has_side_effects);
   }
 
   AddError(

src/tint/resolver/side_effects_test.cc

diff --git a/src/tint/resolver/side_effects_test.cc b/src/tint/resolver/side_effects_test.cc
index 944ff5d..50f4e99 100644
--- a/src/tint/resolver/side_effects_test.cc
+++ b/src/tint/resolver/side_effects_test.cc
@@ -217,7 +217,7 @@
   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 @@
   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()));

src/tint/sem/member_accessor_expression.cc

diff --git a/src/tint/sem/member_accessor_expression.cc b/src/tint/sem/member_accessor_expression.cc
index 26f3842..53b1402 100644
--- a/src/tint/sem/member_accessor_expression.cc
+++ b/src/tint/sem/member_accessor_expression.cc
@@ -46,8 +46,9 @@
 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;

src/tint/sem/member_accessor_expression.h

diff --git a/src/tint/sem/member_accessor_expression.h b/src/tint/sem/member_accessor_expression.h
index df49504..9eed53b 100644
--- a/src/tint/sem/member_accessor_expression.h
+++ b/src/tint/sem/member_accessor_expression.h
@@ -88,10 +88,12 @@
   /// @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;