Fix row-major layout tracking in interface blocks.

Some block field types, such as nested structs, were bugged. This
only affects our "CollectVariables" path, not our current HLSL
UBO path.

BUG=angle:466

Change-Id: I2b8daf58aa7ec1ad06a80d38f57e76087eacccdc
Reviewed-on: https://chromium-review.googlesource.com/213503
Tested-by: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Nicolas Capens <capn@chromium.org>

include/GLSLANG/ShaderVars.h

@@ -87,7 +87,7 @@ struct COMPILER_EXPORT InterfaceBlockField : public ShaderVariable
     InterfaceBlockField(const InterfaceBlockField &other);
     InterfaceBlockField &operator=(const InterfaceBlockField &other);
 
-    bool isRowMajorMatrix;
+    bool isRowMajorLayout;
 };
 
 struct COMPILER_EXPORT Varying : public ShaderVariable

src/common/blocklayout.cpp

@@ -15,62 +15,11 @@ namespace sh
 {
 
 BlockLayoutEncoder::BlockLayoutEncoder()
-    : mCurrentOffset(0),
-      mInRowMajorField(false)
+    : mCurrentOffset(0)
 {
 }
 
-template <typename VarT>
-void BlockLayoutEncoder::encodeVariables(const std::vector<VarT> &fields)
-{
-    for (unsigned int fieldIndex = 0; fieldIndex < fields.size(); fieldIndex++)
-    {
-        const VarT &variable = fields[fieldIndex];
-
-        if (variable.fields.size() > 0)
-        {
-            const unsigned int elementCount = std::max(1u, variable.arraySize);
-
-            for (unsigned int elementIndex = 0; elementIndex < elementCount; elementIndex++)
-            {
-                enterAggregateType();
-                encodeVariables(variable.fields);
-                exitAggregateType();
-            }
-        }
-        else
-        {
-            encodeVariable(variable);
-        }
-    }
-}
-
-// Only defined for interface block fields, and shader variable base
-template void BlockLayoutEncoder::encodeVariables(const std::vector<ShaderVariable> &);
-template void BlockLayoutEncoder::encodeVariables(const std::vector<InterfaceBlockField> &);
-
-BlockMemberInfo BlockLayoutEncoder::encodeVariable(const InterfaceBlockField &field)
-{
-    mInRowMajorField = field.isRowMajorMatrix;
-    return encodeVariable(static_cast<ShaderVariable>(field));
-}
-
-BlockMemberInfo BlockLayoutEncoder::encodeVariable(const sh::ShaderVariable &field)
-{
-    int arrayStride;
-    int matrixStride;
-
-    ASSERT(field.fields.empty());
-    getBlockLayoutInfo(field.type, field.arraySize, mInRowMajorField, &arrayStride, &matrixStride);
-
-    const BlockMemberInfo memberInfo(mCurrentOffset * BytesPerComponent, arrayStride * BytesPerComponent, matrixStride * BytesPerComponent, mInRowMajorField);
-
-    advanceOffset(field.type, field.arraySize, mInRowMajorField, arrayStride, matrixStride);
-
-    return memberInfo;
-}
-
-void BlockLayoutEncoder::encodeType(GLenum type, unsigned int arraySize, bool isRowMajorMatrix)
+BlockMemberInfo BlockLayoutEncoder::encodeType(GLenum type, unsigned int arraySize, bool isRowMajorMatrix)
 {
     int arrayStride;
     int matrixStride;
@@ -80,6 +29,8 @@ void BlockLayoutEncoder::encodeType(GLenum type, unsigned int arraySize, bool is
     const BlockMemberInfo memberInfo(mCurrentOffset * BytesPerComponent, arrayStride * BytesPerComponent, matrixStride * BytesPerComponent, isRowMajorMatrix);
 
     advanceOffset(type, arraySize, isRowMajorMatrix, arrayStride, matrixStride);
+
+    return memberInfo;
 }
 
 void BlockLayoutEncoder::nextRegister()

src/common/blocklayout.h

@@ -49,13 +49,7 @@ class BlockLayoutEncoder
   public:
     BlockLayoutEncoder();
 
-    template <typename VarT>
-    void encodeVariables(const std::vector<VarT> &fields);
-
-    BlockMemberInfo encodeVariable(const sh::ShaderVariable &field);
-    BlockMemberInfo encodeVariable(const InterfaceBlockField &field);
-
-    void encodeType(GLenum type, unsigned int arraySize, bool isRowMajorMatrix);
+    BlockMemberInfo encodeType(GLenum type, unsigned int arraySize, bool isRowMajorMatrix);
 
     size_t getBlockSize() const { return mCurrentOffset * BytesPerComponent; }
     size_t getCurrentRegister() const { return mCurrentOffset / ComponentsPerRegister; }
@@ -69,7 +63,6 @@ class BlockLayoutEncoder
 
   protected:
     size_t mCurrentOffset;
-    bool mInRowMajorField;
 
     void nextRegister();
 

src/compiler/translator/ShaderVars.cpp

@@ -89,7 +89,7 @@ Attribute &Attribute::operator=(const Attribute &other)
 }
 
 InterfaceBlockField::InterfaceBlockField()
-    : isRowMajorMatrix(false)
+    : isRowMajorLayout(false)
 {}
 
 InterfaceBlockField::~InterfaceBlockField()
@@ -97,13 +97,13 @@ InterfaceBlockField::~InterfaceBlockField()
 
 InterfaceBlockField::InterfaceBlockField(const InterfaceBlockField &other)
     : ShaderVariable(other),
-      isRowMajorMatrix(other.isRowMajorMatrix)
+      isRowMajorLayout(other.isRowMajorLayout)
 {}
 
 InterfaceBlockField &InterfaceBlockField::operator=(const InterfaceBlockField &other)
 {
     ShaderVariable::operator=(other);
-    isRowMajorMatrix = other.isRowMajorMatrix;
+    isRowMajorLayout = other.isRowMajorLayout;
     return *this;
 }
 

src/compiler/translator/VariableInfo.cpp

@@ -272,6 +272,7 @@ void CollectVariables::visitVariable(const TIntermSymbol *variable,
     interfaceBlock.isRowMajorLayout = (blockType->matrixPacking() == EmpRowMajor);
     interfaceBlock.layout = sh::GetBlockLayoutType(blockType->blockStorage());
 
+    // Gather field information
     sh::GetInterfaceBlockFields(*blockType, &interfaceBlock.fields);
 
     infoList->push_back(interfaceBlock);
@@ -356,13 +357,14 @@ bool CollectVariables::visitBinary(Visit, TIntermBinary *binaryNode)
 {
     if (binaryNode->getOp() == EOpIndexDirectInterfaceBlock)
     {
-        TIntermSymbol *symbol = binaryNode->getLeft()->getAsSymbolNode();
-        ASSERT(symbol);
+        // NOTE: we do not determine static use for individual blocks of an array
+        TIntermTyped *blockNode = binaryNode->getLeft()->getAsTyped();
+        ASSERT(blockNode);
 
         TIntermConstantUnion *constantUnion = binaryNode->getRight()->getAsConstantUnion();
         ASSERT(constantUnion);
 
-        const TInterfaceBlock *interfaceBlock = symbol->getType().getInterfaceBlock();
+        const TInterfaceBlock *interfaceBlock = blockNode->getType().getInterfaceBlock();
         sh::InterfaceBlock *namedBlock = FindVariable(interfaceBlock->name(), mInterfaceBlocks);
         ASSERT(namedBlock);
         namedBlock->staticUse = true;

src/compiler/translator/util.cpp

@@ -356,7 +356,7 @@ void GetInterfaceBlockFields(const TInterfaceBlock &interfaceBlock, std::vector<
         GetVariableTraverser traverser;
         traverser.traverse(fieldType, fullFieldName, fieldsOut);
 
-        fieldsOut->back().isRowMajorMatrix = (fieldType.getLayoutQualifier().matrixPacking == EmpRowMajor);
+        fieldsOut->back().isRowMajorLayout = (fieldType.getLayoutQualifier().matrixPacking == EmpRowMajor);
     }
 }
 

src/libGLESv2/ProgramBinary.cpp

@@ -110,6 +110,16 @@ void GetInputLayoutFromShader(const std::vector<sh::Attribute> &shaderAttributes
     }
 }
 
+bool IsRowMajorLayout(const sh::InterfaceBlockField &var)
+{
+    return var.isRowMajorLayout;
+}
+
+bool IsRowMajorLayout(const sh::ShaderVariable &var)
+{
+    return false;
+}
+
 }
 
 VariableLocation::VariableLocation(const std::string &name, unsigned int element, unsigned int index)
@@ -1879,7 +1889,7 @@ bool ProgramBinary::linkValidateInterfaceBlockFields(InfoLog &infoLog, const std
         return false;
     }
 
-    if (vertexUniform.isRowMajorMatrix != fragmentUniform.isRowMajorMatrix)
+    if (vertexUniform.isRowMajorLayout != fragmentUniform.isRowMajorLayout)
     {
         infoLog.append("Matrix packings for %s differ between vertex and fragment shaders", uniformName.c_str());
         return false;
@@ -2273,7 +2283,8 @@ bool ProgramBinary::gatherTransformFeedbackLinkedVaryings(InfoLog &infoLog, cons
 
 template <typename VarT>
 void ProgramBinary::defineUniformBlockMembers(const std::vector<VarT> &fields, const std::string &prefix, int blockIndex,
-                                              sh::BlockLayoutEncoder *encoder, std::vector<unsigned int> *blockUniformIndexes)
+                                              sh::BlockLayoutEncoder *encoder, std::vector<unsigned int> *blockUniformIndexes,
+                                              bool inRowMajorLayout)
 {
     for (unsigned int uniformIndex = 0; uniformIndex < fields.size(); uniformIndex++)
     {
@@ -2282,19 +2293,23 @@ void ProgramBinary::defineUniformBlockMembers(const std::vector<VarT> &fields, c
 
         if (field.isStruct())
         {
+            bool rowMajorLayout = (inRowMajorLayout || IsRowMajorLayout(field));
+
             for (unsigned int arrayElement = 0; arrayElement < field.elementCount(); arrayElement++)
             {
                 encoder->enterAggregateType();
 
                 const std::string uniformElementName = fieldName + (field.isArray() ? ArrayString(arrayElement) : "");
-                defineUniformBlockMembers(field.fields, uniformElementName, blockIndex, encoder, blockUniformIndexes);
+                defineUniformBlockMembers(field.fields, uniformElementName, blockIndex, encoder, blockUniformIndexes, rowMajorLayout);
 
                 encoder->exitAggregateType();
             }
         }
         else
         {
-            sh::BlockMemberInfo memberInfo = encoder->encodeVariable(field);
+            bool isRowMajorMatrix = (IsMatrixType(field.type) && inRowMajorLayout);
+
+            sh::BlockMemberInfo memberInfo = encoder->encodeType(field.type, field.arraySize, isRowMajorMatrix);
 
             LinkedUniform *newUniform = new LinkedUniform(field.type, field.precision, fieldName, field.arraySize,
                                                           blockIndex, memberInfo);
@@ -2329,7 +2344,7 @@ bool ProgramBinary::defineUniformBlock(InfoLog &infoLog, const Shader &shader, c
         }
         ASSERT(encoder);
 
-        defineUniformBlockMembers(interfaceBlock.fields, "", blockIndex, encoder, &blockUniformIndexes);
+        defineUniformBlockMembers(interfaceBlock.fields, "", blockIndex, encoder, &blockUniformIndexes, interfaceBlock.isRowMajorLayout);
 
         size_t dataSize = encoder->getBlockSize();
 

src/libGLESv2/ProgramBinary.h

@@ -221,7 +221,8 @@ class ProgramBinary : public RefCountObject
                                                std::vector<LinkedVarying> *outTransformFeedbackLinkedVaryings) const;
     template <typename VarT>
     void defineUniformBlockMembers(const std::vector<VarT> &fields, const std::string &prefix, int blockIndex,
-                                   sh::BlockLayoutEncoder *encoder, std::vector<unsigned int> *blockUniformIndexes);
+                                   sh::BlockLayoutEncoder *encoder, std::vector<unsigned int> *blockUniformIndexes,
+                                   bool inRowMajorLayout);
     bool defineUniformBlock(InfoLog &infoLog, const Shader &shader, const sh::InterfaceBlock &interfaceBlock);
     bool assignUniformBlockRegister(InfoLog &infoLog, UniformBlock *uniformBlock, GLenum shader, unsigned int registerIndex);
     void defineOutputVariables(FragmentShader *fragmentShader);

tests/compiler_tests/CollectVariables_test.cpp

@@ -164,7 +164,7 @@ TEST_F(CollectVertexVariablesTest, SimpleInterfaceBlock)
     EXPECT_TRUE(field.staticUse);
     EXPECT_GLENUM_EQ(GL_FLOAT, field.type);
     EXPECT_EQ("f", field.name);
-    EXPECT_FALSE(field.isRowMajorMatrix);
+    EXPECT_FALSE(field.isRowMajorLayout);
     EXPECT_TRUE(field.fields.empty());
 }
 
@@ -201,7 +201,7 @@ TEST_F(CollectVertexVariablesTest, SimpleInstancedInterfaceBlock)
     EXPECT_TRUE(field.staticUse);
     EXPECT_GLENUM_EQ(GL_FLOAT, field.type);
     EXPECT_EQ("b.f", field.name);
-    EXPECT_FALSE(field.isRowMajorMatrix);
+    EXPECT_FALSE(field.isRowMajorLayout);
     EXPECT_TRUE(field.fields.empty());
 }
 
@@ -238,7 +238,7 @@ TEST_F(CollectVertexVariablesTest, StructInterfaceBlock)
     EXPECT_TRUE(field.isStruct());
     EXPECT_TRUE(field.staticUse);
     EXPECT_EQ("s", field.name);
-    EXPECT_FALSE(field.isRowMajorMatrix);
+    EXPECT_FALSE(field.isRowMajorLayout);
 
     const sh::ShaderVariable &member = field.fields[0];
 
@@ -282,7 +282,7 @@ TEST_F(CollectVertexVariablesTest, StructInstancedInterfaceBlock)
     EXPECT_TRUE(field.isStruct());
     EXPECT_TRUE(field.staticUse);
     EXPECT_EQ("b.s", field.name);
-    EXPECT_FALSE(field.isRowMajorMatrix);
+    EXPECT_FALSE(field.isRowMajorLayout);
 
     const sh::ShaderVariable &member = field.fields[0];
 
@@ -292,3 +292,47 @@ TEST_F(CollectVertexVariablesTest, StructInstancedInterfaceBlock)
     EXPECT_GLENUM_EQ(GL_FLOAT, member.type);
     EXPECT_GLENUM_EQ(GL_HIGH_FLOAT, member.precision);
 }
+
+TEST_F(CollectVertexVariablesTest, NestedStructRowMajorInterfaceBlock)
+{
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "struct st { mat2 m; };"
+        "layout(row_major) uniform b {\n"
+        "  st s;\n"
+        "};"
+        "void main() {\n"
+        "   gl_Position = vec4(s.m);\n"
+        "}\n";
+
+    const char *shaderStrings[] = { shaderString.c_str() };
+    ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES));
+
+    const std::vector<sh::InterfaceBlock> &interfaceBlocks = mTranslator->getInterfaceBlocks();
+    ASSERT_EQ(1u, interfaceBlocks.size());
+
+    const sh::InterfaceBlock &interfaceBlock = interfaceBlocks[0];
+
+    EXPECT_EQ(0u, interfaceBlock.arraySize);
+    EXPECT_TRUE(interfaceBlock.isRowMajorLayout);
+    EXPECT_EQ(sh::BLOCKLAYOUT_SHARED, interfaceBlock.layout);
+    EXPECT_EQ("b", interfaceBlock.name);
+    EXPECT_TRUE(interfaceBlock.staticUse);
+
+    ASSERT_EQ(1u, interfaceBlock.fields.size());
+
+    const sh::InterfaceBlockField &field = interfaceBlock.fields[0];
+
+    EXPECT_TRUE(field.isStruct());
+    EXPECT_TRUE(field.staticUse);
+    EXPECT_EQ("s", field.name);
+    EXPECT_TRUE(field.isRowMajorLayout);
+
+    const sh::ShaderVariable &member = field.fields[0];
+
+    // NOTE: we don't currently mark struct members as statically used or not
+    EXPECT_FALSE(member.isStruct());
+    EXPECT_EQ("m", member.name);
+    EXPECT_GLENUM_EQ(GL_FLOAT_MAT2, member.type);
+    EXPECT_GLENUM_EQ(GL_HIGH_FLOAT, member.precision);
+}