From 4be13e30b547e9e77113824aa36557370a45c2cc Mon Sep 17 00:00:00 2001
From: Shahbaz Youssefi <[email protected]>
Date: Fri, 22 May 2026 11:45:18 -0400
Subject: [PATCH] Translator: Prune comma expressions more aggressively

With this change, `(a[side_effect].b, c)` is transformed to
`(side_effect, c)`.  Also expressions such as `(side_effect, b, c)` are
transformed to `(side_effect, c)`.

Bug: chromium:500030250
Bug: chromium:518243653
Change-Id: I7c2f853f0f39b246f44bf4ad383115dfcab31309
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/7872078
Commit-Queue: Shahbaz Youssefi <[email protected]>
Reviewed-by: Geoff Lang <[email protected]>
Reviewed-by: Yuxin Hu <[email protected]>
---

diff --git a/src/compiler/translator/tree_ops/PruneNoOps.cpp b/src/compiler/translator/tree_ops/PruneNoOps.cpp
index 6e832c1..0895324 100644
--- a/src/compiler/translator/tree_ops/PruneNoOps.cpp
+++ b/src/compiler/translator/tree_ops/PruneNoOps.cpp
@@ -99,6 +99,15 @@
     return !node->getAsTyped()->hasSideEffects();
 }
 
+enum class CommaExpression
+{
+    // The LHS of the comma expression will get thrown away, so it can be decomposed and its side
+    // effects extracted
+    ThrowAway,
+    // The RHS of the comma expression needs to be retained as its result is used.
+    FinalResult,
+};
+
 class PruneNoOpsTraverser : private TIntermTraverser
 {
   public:
@@ -111,9 +120,12 @@
     bool visitDeclaration(Visit, TIntermDeclaration *node) override;
     bool visitSwitch(Visit visit, TIntermSwitch *node) override;
     bool visitBlock(Visit visit, TIntermBlock *node) override;
+    bool visitBinary(Visit visit, TIntermBinary *node) override;
     bool visitLoop(Visit visit, TIntermLoop *loop) override;
     bool visitBranch(Visit visit, TIntermBranch *node) override;
-    TIntermTyped *pruneNoOpCommaExpressions(TIntermTyped *statement);
+    TIntermTyped *pruneCommaThrowAwayExpression(TIntermTyped *statement);
+    TIntermTyped *pruneNoOpCommaExpressions(TIntermTyped *statement, CommaExpression commaExpr);
+    TIntermTyped *mergePrunedNoOpCommaExpressions(TIntermTyped *lhs, TIntermTyped *rhs);
 
     bool mIsBranchVisited = false;
 
@@ -129,15 +141,12 @@
 }
 
 PruneNoOpsTraverser::PruneNoOpsTraverser(TSymbolTable *symbolTable)
-    : TIntermTraverser(true, true, true, symbolTable)
+    : TIntermTraverser(true, false, false, symbolTable)
 {}
 
 bool PruneNoOpsTraverser::visitDeclaration(Visit visit, TIntermDeclaration *node)
 {
-    if (visit != PreVisit)
-    {
-        return true;
-    }
+    ASSERT(visit == PreVisit);
 
     TIntermSequence *sequence = node->getSequence();
     if (sequence->size() >= 1)
@@ -195,9 +204,10 @@
                 queueReplacementWithParent(node, declaratorSymbol, new TIntermSymbol(variable),
                                            OriginalNode::IS_DROPPED);
             }
+            return false;
         }
     }
-    return false;
+    return true;
 }
 
 bool PruneNoOpsTraverser::visitSwitch(Visit visit, TIntermSwitch *node)
@@ -295,7 +305,8 @@
         // the ones with side effect back together with comma.
         if (statement->getAsBinaryNode() != nullptr)
         {
-            statement = pruneNoOpCommaExpressions(statement->getAsBinaryNode());
+            statement = pruneNoOpCommaExpressions(statement->getAsBinaryNode(),
+                                                  CommaExpression::FinalResult);
             if (statement == nullptr)
             {
                 continue;
@@ -319,19 +330,93 @@
     return false;
 }
 
-TIntermTyped *PruneNoOpsTraverser::pruneNoOpCommaExpressions(TIntermTyped *statement)
+bool PruneNoOpsTraverser::visitBinary(Visit visit, TIntermBinary *node)
+{
+    if (node->getOp() == EOpComma && getParentNode()->getAsBlock() == nullptr)
+    {
+        // Prune LHS of the comma.  This is not done if the parent is a block node because
+        // visitBlock() already does it.
+        TIntermTyped *prunedLeft = pruneCommaThrowAwayExpression(node->getLeft());
+        if (prunedLeft != node->getLeft())
+        {
+            // If completely pruned, replace with RHS, otherwise replace the LHS with its side
+            // effects.
+            queueReplacement(prunedLeft != nullptr
+                                 ? new TIntermBinary(EOpComma, prunedLeft, node->getRight())
+                                 : node->getRight(),
+                             OriginalNode::IS_DROPPED);
+
+            node->getRight()->traverse(this);
+            if (prunedLeft)
+            {
+                prunedLeft->traverse(this);
+            }
+            return false;
+        }
+    }
+
+    return true;
+}
+
+TIntermTyped *PruneNoOpsTraverser::pruneCommaThrowAwayExpression(TIntermTyped *statement)
+{
+    if (IsNoOp(statement))
+    {
+        return nullptr;
+    }
+
+    TIntermBinary *asBinary = statement->getAsBinaryNode();
+    if (asBinary == nullptr)
+    {
+        return statement;
+    }
+
+    switch (asBinary->getOp())
+    {
+        case EOpIndexDirect:
+        case EOpIndexDirectStruct:
+        case EOpIndexDirectInterfaceBlock:
+            return pruneNoOpCommaExpressions(asBinary->getLeft(), CommaExpression::ThrowAway);
+        case EOpIndexIndirect:
+        case EOpComma:
+        {
+            // Prune both the indexed and the indexee.  If both have side effects, join them with a
+            // comma.
+            //
+            // Same with a comma operation's left and right hand side expressions.  Since
+            // |statement| is itself the LHS of a comma operation, both its LHS and RHS can be
+            // pruned.
+            TIntermTyped *prunedLeft =
+                pruneNoOpCommaExpressions(asBinary->getLeft(), CommaExpression::ThrowAway);
+            TIntermTyped *prunedRight =
+                pruneNoOpCommaExpressions(asBinary->getRight(), CommaExpression::ThrowAway);
+            return mergePrunedNoOpCommaExpressions(prunedLeft, prunedRight);
+        }
+        default:
+            return statement;
+    }
+}
+
+TIntermTyped *PruneNoOpsTraverser::pruneNoOpCommaExpressions(TIntermTyped *statement,
+                                                             CommaExpression commaExpr)
 {
     TIntermBinary *commaSeparatedExpressions = statement->getAsBinaryNode();
     if (commaSeparatedExpressions == nullptr || commaSeparatedExpressions->getOp() != EOpComma)
     {
-        return statement;
+        // If this is not the final result of comma, try to extract side effect out of the
+        // expression and throw the rest away.  In an expression like
+        // |struct_with_sampler[side_effect]|, this allows it to be replaced by |side_effect| alone.
+        return commaExpr == CommaExpression::ThrowAway ? pruneCommaThrowAwayExpression(statement)
+                                                       : statement;
     }
 
     TIntermTyped *left  = commaSeparatedExpressions->getLeft();
     TIntermTyped *right = commaSeparatedExpressions->getRight();
 
-    TIntermTyped *prunedLeft  = IsNoOp(left) ? nullptr : pruneNoOpCommaExpressions(left);
-    TIntermTyped *prunedRight = IsNoOp(right) ? nullptr : pruneNoOpCommaExpressions(right);
+    TIntermTyped *prunedLeft =
+        IsNoOp(left) ? nullptr : pruneNoOpCommaExpressions(left, CommaExpression::ThrowAway);
+    TIntermTyped *prunedRight =
+        IsNoOp(right) ? nullptr : pruneNoOpCommaExpressions(right, commaExpr);
 
     if (left == prunedLeft && right == prunedRight)
     {
@@ -339,26 +424,29 @@
         return statement;
     }
 
+    return mergePrunedNoOpCommaExpressions(prunedLeft, prunedRight);
+}
+
+TIntermTyped *PruneNoOpsTraverser::mergePrunedNoOpCommaExpressions(TIntermTyped *lhs,
+                                                                   TIntermTyped *rhs)
+{

diff --git a/src/tests/angle_end2end_tests_expectations.txt b/src/tests/angle_end2end_tests_expectations.txt
index 9f95376..1090127 100644
--- a/src/tests/angle_end2end_tests_expectations.txt
+++ b/src/tests/angle_end2end_tests_expectations.txt
@@ -428,6 +428,7 @@
 497533569 MAC INTEL OPENGL : TransformFeedbackTest.ProgramSwitchDuringPauseAndResume/* = SKIP
 497533569 MAC INTEL OPENGL : TransformFeedbackTest.ProgramSwitchDuringPauseAndResumeWithBufferChange/* = SKIP
 513172707 MAC INTEL OPENGL : DrawBaseVertexBaseInstanceTest_ES3.InstanceIDDoesNotIncludeBaseInstance/* = SKIP
+519471062 MAC OPENGL : GLSLTest_ES3.SamplerInStructRHSOfCommaWithSideEffect/* = SKIP
 
 // BlitFramebufferTest.ScissoredMultisampleStencil failures
 42262159 MAC INTEL OPENGL : BlitFramebufferTest.ScissoredMultisampleStencil/* = SKIP
diff --git a/src/tests/gl_tests/GLSLTest.cpp b/src/tests/gl_tests/GLSLTest.cpp
index 79ed94c..cd8223f 100644
--- a/src/tests/gl_tests/GLSLTest.cpp
+++ b/src/tests/gl_tests/GLSLTest.cpp
@@ -6997,6 +6997,41 @@
 }
 
 // Test that samplers in structs can be used on the right-hand side of a comma, where the expression
+// has side effect.
+TEST_P(GLSLTest_ES3, SamplerInStructRHSOfCommaWithSideEffect)
+{
+    constexpr char kFS[] = R"(#version 300 es
+precision mediump float;
+uniform struct {
+    sampler2D n;
+    vec2 c;
+} s[4];
+ivec4 global = ivec4(0);
+out vec4 color;
+void main()
+{
+    int i = 0;
+    (s, s[i += 1].n), (global.x = 10);
+    s[i += 2], s[i].c, (s[i - 1].n, (global.y = 20));
+    s[global.w = 80, i += 4].c, (global.z = 40);
+
+    int c11 = ((s, s[i += 8].n), (global.x += 1));
+    int c22 = (s[i += 16], s[i].c, (s[i - 1].n, (global.y += 2)));
+    int c43 = (s[global.w += 4, i += 32].c, (global.z += 3));
+    vec4 allOnes = vec4(((i += 64, s), 1.0));
+
+    color = vec4(i == 127,
+                 global.x == 11 && global.y == 22,
+                 global.z == 43 && global.w == 84,
+                 c11 == 11 && c22 == 22 && c43 == 43) * allOnes;
+})";
+
+    ANGLE_GL_PROGRAM(program, essl3_shaders::vs::Simple(), kFS);
+    drawQuad(program, essl3_shaders::PositionAttrib(), 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::white);
+}
+
+// Test that samplers in structs can be used on the right-hand side of a comma, where the expression
 // has side effect, and that the struct field can be selected on the comma expression.
 TEST_P(GLSLTest_ES3, SamplerInStructRHSOfCommaWithSideEffectWithSelectField)
 {