Clean up temporary variable usage in ScalarizeVecAndMatConstructorArgs

src/compiler/translator/Compiler.cpp

@@ -438,9 +438,8 @@ TIntermBlock *TCompiler::compileTreeImpl(const char *const shaderStrings[],
 
         if (success && (compileOptions & SH_SCALARIZE_VEC_AND_MAT_CONSTRUCTOR_ARGS))
         {
-            ScalarizeVecAndMatConstructorArgs scalarizer(
-                shaderType, fragmentPrecisionHigh);
-            root->traverse(&scalarizer);
+            ScalarizeVecAndMatConstructorArgs(root, shaderType, fragmentPrecisionHigh,
+                                              &mTemporaryIndex);
         }
 
         if (success && (compileOptions & SH_REGENERATE_STRUCT_NAMES))

src/compiler/translator/IntermTraverse.cpp

@@ -151,7 +151,11 @@ TIntermSymbol *TIntermTraverser::createTempSymbol(const TType &type, TQualifier
 
     TIntermSymbol *node = new TIntermSymbol(0, symbolName, type);
     node->setInternal(true);
+
+    ASSERT(qualifier == EvqTemporary || qualifier == EvqConst || qualifier == EvqGlobal);
     node->getTypePointer()->setQualifier(qualifier);
+    // TODO(oetuaho): Might be useful to sanitize layout qualifier etc. on the type of the created
+    // symbol. This might need to be done in other places as well.
     return node;
 }
 

src/compiler/translator/ScalarizeVecAndMatConstructorArgs.cpp

@@ -3,6 +3,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
+// Scalarize vector and matrix constructor args, so that vectors built from components don't have
+// matrix arguments, and matrices built from components don't have vector arguments. This avoids
+// driver bugs around vector and matrix constructors.
+//
 
 #include "common/debug.h"
 #include "compiler/translator/ScalarizeVecAndMatConstructorArgs.h"
@@ -11,6 +15,7 @@
 
 #include "angle_gl.h"
 #include "common/angleutils.h"
+#include "compiler/translator/IntermNode.h"
 
 namespace
 {
@@ -52,9 +57,44 @@ TIntermBinary *ConstructMatrixIndexBinaryNode(
                              TIntermTyped::CreateIndexNode(rowIndex));
 }
 
-}  // namespace anonymous
+class ScalarizeArgsTraverser : public TIntermTraverser
+{
+  public:
+    ScalarizeArgsTraverser(sh::GLenum shaderType,
+                           bool fragmentPrecisionHigh,
+                           unsigned int *temporaryIndex)
+        : TIntermTraverser(true, false, false),
+          mShaderType(shaderType),
+          mFragmentPrecisionHigh(fragmentPrecisionHigh)
+    {
+        useTemporaryIndex(temporaryIndex);
+    }
+
+  protected:
+    bool visitAggregate(Visit visit, TIntermAggregate *node) override;
+    bool visitBlock(Visit visit, TIntermBlock *node) override;
+
+  private:
+    void scalarizeArgs(TIntermAggregate *aggregate, bool scalarizeVector, bool scalarizeMatrix);
 
-bool ScalarizeVecAndMatConstructorArgs::visitAggregate(Visit visit, TIntermAggregate *node)
+    // If we have the following code:
+    //   mat4 m(0);
+    //   vec4 v(1, m);
+    // We will rewrite to:
+    //   mat4 m(0);
+    //   mat4 s0 = m;
+    //   vec4 v(1, s0[0][0], s0[0][1], s0[0][2]);
+    // This function is to create nodes for "mat4 s0 = m;" and insert it to the code sequence. This
+    // way the possible side effects of the constructor argument will only be evaluated once.
+    void createTempVariable(TIntermTyped *original);
+
+    std::vector<TIntermSequence> mBlockStack;
+
+    sh::GLenum mShaderType;
+    bool mFragmentPrecisionHigh;
+};
+
+bool ScalarizeArgsTraverser::visitAggregate(Visit visit, TIntermAggregate *node)
 {
     if (visit == PreVisit)
     {
@@ -91,7 +131,7 @@ bool ScalarizeVecAndMatConstructorArgs::visitAggregate(Visit visit, TIntermAggre
     return true;
 }
 
-bool ScalarizeVecAndMatConstructorArgs::visitBlock(Visit visit, TIntermBlock *node)
+bool ScalarizeArgsTraverser::visitBlock(Visit visit, TIntermBlock *node)
 {
     mBlockStack.push_back(TIntermSequence());
     {
@@ -111,8 +151,9 @@ bool ScalarizeVecAndMatConstructorArgs::visitBlock(Visit visit, TIntermBlock *no
     return false;
 }
 
-void ScalarizeVecAndMatConstructorArgs::scalarizeArgs(
-    TIntermAggregate *aggregate, bool scalarizeVector, bool scalarizeMatrix)
+void ScalarizeArgsTraverser::scalarizeArgs(TIntermAggregate *aggregate,
+                                           bool scalarizeVector,
+                                           bool scalarizeMatrix)
 {
     ASSERT(aggregate);
     int size = 0;
@@ -163,12 +204,10 @@ void ScalarizeVecAndMatConstructorArgs::scalarizeArgs(
         ASSERT(size > 0);
         TIntermTyped *node = original[ii]->getAsTyped();
         ASSERT(node);
-        TString varName = createTempVariable(node);
+        createTempVariable(node);
         if (node->isScalar())
         {
-            TIntermSymbol *symbolNode =
-                new TIntermSymbol(-1, varName, node->getType());
-            sequence->push_back(symbolNode);
+            sequence->push_back(createTempSymbol(node->getType()));
             size--;
         }
         else if (node->isVector())
@@ -179,8 +218,7 @@ void ScalarizeVecAndMatConstructorArgs::scalarizeArgs(
                 size -= repeat;
                 for (int index = 0; index < repeat; ++index)
                 {
-                    TIntermSymbol *symbolNode =
-                        new TIntermSymbol(-1, varName, node->getType());
+                    TIntermSymbol *symbolNode = createTempSymbol(node->getType());
                     TIntermBinary *newNode = ConstructVectorIndexBinaryNode(
                         symbolNode, index);
                     sequence->push_back(newNode);
@@ -188,8 +226,7 @@ void ScalarizeVecAndMatConstructorArgs::scalarizeArgs(
             }
             else
             {
-                TIntermSymbol *symbolNode =
-                    new TIntermSymbol(-1, varName, node->getType());
+                TIntermSymbol *symbolNode = createTempSymbol(node->getType());
                 sequence->push_back(symbolNode);
                 size -= node->getNominalSize();
             }
@@ -204,8 +241,7 @@ void ScalarizeVecAndMatConstructorArgs::scalarizeArgs(
                 size -= repeat;
                 while (repeat > 0)
                 {
-                    TIntermSymbol *symbolNode =
-                        new TIntermSymbol(-1, varName, node->getType());
+                    TIntermSymbol *symbolNode = createTempSymbol(node->getType());
                     TIntermBinary *newNode = ConstructMatrixIndexBinaryNode(
                         symbolNode, colIndex, rowIndex);
                     sequence->push_back(newNode);
@@ -220,8 +256,7 @@ void ScalarizeVecAndMatConstructorArgs::scalarizeArgs(
             }
             else
             {
-                TIntermSymbol *symbolNode =
-                    new TIntermSymbol(-1, varName, node->getType());
+                TIntermSymbol *symbolNode = createTempSymbol(node->getType());
                 sequence->push_back(symbolNode);
                 size -= node->getCols() * node->getRows();
             }
@@ -229,29 +264,13 @@ void ScalarizeVecAndMatConstructorArgs::scalarizeArgs(
     }
 }
 
-TString ScalarizeVecAndMatConstructorArgs::createTempVariable(TIntermTyped *original)
+void ScalarizeArgsTraverser::createTempVariable(TIntermTyped *original)
 {
-    TString tempVarName = "_webgl_tmp_";
-    if (original->isScalar())
-    {
-        tempVarName += "scalar_";
-    }
-    else if (original->isVector())
-    {
-        tempVarName += "vec_";
-    }
-    else
-    {
-        ASSERT(original->isMatrix());
-        tempVarName += "mat_";
-    }
-    tempVarName += Str(mTempVarCount).c_str();
-    mTempVarCount++;
-
     ASSERT(original);
-    TType type = original->getType();
-    type.setQualifier(EvqTemporary);
+    nextTemporaryIndex();
+    TIntermDeclaration *decl = createTempInitDeclaration(original);
 
+    TType type = original->getType();
     if (mShaderType == GL_FRAGMENT_SHADER &&
         type.getBasicType() == EbtFloat &&
         type.getPrecision() == EbpUndefined)
@@ -259,18 +278,24 @@ TString ScalarizeVecAndMatConstructorArgs::createTempVariable(TIntermTyped *orig
         // We use the highest available precision for the temporary variable
         // to avoid computing the actual precision using the rules defined
         // in GLSL ES 1.0 Section 4.5.2.
-        type.setPrecision(mFragmentPrecisionHigh ? EbpHigh : EbpMedium);
+        TIntermBinary *init = decl->getSequence()->at(0)->getAsBinaryNode();
+        init->getTypePointer()->setPrecision(mFragmentPrecisionHigh ? EbpHigh : EbpMedium);
+        init->getLeft()->getTypePointer()->setPrecision(mFragmentPrecisionHigh ? EbpHigh
+                                                                               : EbpMedium);
     }
 
-    TIntermSymbol *symbolNode = new TIntermSymbol(-1, tempVarName, type);
-    TIntermBinary *init       = new TIntermBinary(EOpInitialize, symbolNode, original);
-
-    TIntermDeclaration *decl = new TIntermDeclaration();
-    decl->appendDeclarator(init);
-
     ASSERT(mBlockStack.size() > 0);
     TIntermSequence &sequence = mBlockStack.back();
     sequence.push_back(decl);
-
-    return tempVarName;
 }
+
+}  // namespace anonymous
+
+void ScalarizeVecAndMatConstructorArgs(TIntermBlock *root,
+                                       sh::GLenum shaderType,
+                                       bool fragmentPrecisionHigh,
+                                       unsigned int *temporaryIndex)
+{
+    ScalarizeArgsTraverser scalarizer(shaderType, fragmentPrecisionHigh, temporaryIndex);
+    root->traverse(&scalarizer);
+}
\ No newline at end of file

src/compiler/translator/ScalarizeVecAndMatConstructorArgs.h

@@ -3,47 +3,21 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
+// Scalarize vector and matrix constructor args, so that vectors built from components don't have
+// matrix arguments, and matrices built from components don't have vector arguments. This avoids
+// driver bugs around vector and matrix constructors.
+//
 
 #ifndef COMPILER_TRANSLATOR_SCALARIZEVECANDMATCONSTRUCTORARGS_H_
 #define COMPILER_TRANSLATOR_SCALARIZEVECANDMATCONSTRUCTORARGS_H_
 
-#include "compiler/translator/IntermNode.h"
-
-class ScalarizeVecAndMatConstructorArgs : public TIntermTraverser
-{
-  public:
-    ScalarizeVecAndMatConstructorArgs(sh::GLenum shaderType,
-                                      bool fragmentPrecisionHigh)
-        : TIntermTraverser(true, false, false),
-          mTempVarCount(0),
-          mShaderType(shaderType),
-          mFragmentPrecisionHigh(fragmentPrecisionHigh) {}
-
-  protected:
-    bool visitAggregate(Visit visit, TIntermAggregate *node) override;
-    bool visitBlock(Visit visit, TIntermBlock *node) override;
-
-  private:
-    void scalarizeArgs(TIntermAggregate *aggregate,
-                       bool scalarizeVector, bool scalarizeMatrix);
-
-    // If we have the following code:
-    //   mat4 m(0);
-    //   vec4 v(1, m);
-    // We will rewrite to:
-    //   mat4 m(0);
-    //   mat4 _webgl_tmp_mat_0 = m;
-    //   vec4 v(1, _webgl_tmp_mat_0[0][0], _webgl_tmp_mat_0[0][1], _webgl_tmp_mat_0[0][2]);
-    // This function is to create nodes for "mat4 _webgl_tmp_mat_0 = m;" and insert it to
-    // the code sequence.
-    // Return the temporary variable name.
-    TString createTempVariable(TIntermTyped *original);
+#include "GLSLANG/ShaderLang.h"
 
-    std::vector<TIntermSequence> mBlockStack;
-    int mTempVarCount;
+class TIntermBlock;
 
-    sh::GLenum mShaderType;
-    bool mFragmentPrecisionHigh;
-};
+void ScalarizeVecAndMatConstructorArgs(TIntermBlock *root,
+                                       sh::GLenum shaderType,
+                                       bool fragmentPrecisionHigh,
+                                       unsigned int *temporaryIndex);
 
 #endif  // COMPILER_TRANSLATOR_SCALARIZEVECANDMATCONSTRUCTORARGS_H_