Fix writing uniform block maps to HLSL output

src/compiler/translator/FlagStd140Structs.cpp

@@ -3,75 +3,73 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
+// FlagStd140Structs.cpp: Find structs in std140 blocks, where the padding added in the translator
+// conflicts with the "natural" unpadded type.
 
 #include "compiler/translator/FlagStd140Structs.h"
 
+#include "compiler/translator/IntermTraverse.h"
+
 namespace sh
 {
 
-bool FlagStd140Structs::visitBinary(Visit visit, TIntermBinary *binaryNode)
+namespace
 {
-    if (binaryNode->getRight()->getBasicType() == EbtStruct)
-    {
-        switch (binaryNode->getOp())
-        {
-            case EOpIndexDirectInterfaceBlock:
-            case EOpIndexDirectStruct:
-                if (isInStd140InterfaceBlock(binaryNode->getLeft()))
-                {
-                    mFlaggedNodes.push_back(binaryNode);
-                }
-                break;
 
-            default:
-                break;
-        }
-        return false;
-    }
+class FlagStd140StructsTraverser : public TIntermTraverser
+{
+  public:
+    FlagStd140StructsTraverser() : TIntermTraverser(true, false, false) {}
 
-    if (binaryNode->getOp() == EOpIndexDirectStruct)
-    {
-        return false;
-    }
+    const std::vector<MappedStruct> getMappedStructs() const { return mMappedStructs; }
 
-    return visit == PreVisit;
-}
+  protected:
+    bool visitDeclaration(Visit visit, TIntermDeclaration *node) override;
 
-void FlagStd140Structs::visitSymbol(TIntermSymbol *symbol)
+  private:
+    void mapBlockStructMembers(TIntermSymbol *blockDeclarator, TInterfaceBlock *block);
+
+    std::vector<MappedStruct> mMappedStructs;
+};
+
+void FlagStd140StructsTraverser::mapBlockStructMembers(TIntermSymbol *blockDeclarator,
+                                                       TInterfaceBlock *block)
 {
-    if (isInStd140InterfaceBlock(symbol) && symbol->getBasicType() == EbtStruct)
+    for (auto *field : block->fields())
     {
-        mFlaggedNodes.push_back(symbol);
+        if (field->type()->getBasicType() == EbtStruct)
+        {
+            MappedStruct mappedStruct;
+            mappedStruct.blockDeclarator = blockDeclarator;
+            mappedStruct.field           = field;
+            mMappedStructs.push_back(mappedStruct);
+        }
     }
 }
 
-bool FlagStd140Structs::isInStd140InterfaceBlock(TIntermTyped *node) const
+bool FlagStd140StructsTraverser::visitDeclaration(Visit visit, TIntermDeclaration *node)
 {
-    TIntermBinary *binaryNode = node->getAsBinaryNode();
-
-    if (binaryNode)
+    TIntermTyped *declarator = node->getSequence()->back()->getAsTyped();
+    if (declarator->getBasicType() == EbtInterfaceBlock)
     {
-        return isInStd140InterfaceBlock(binaryNode->getLeft());
-    }
-
-    const TType &type = node->getType();
-
-    // determine if we are in the standard layout
-    const TInterfaceBlock *interfaceBlock = type.getInterfaceBlock();
-    if (interfaceBlock)
+        TInterfaceBlock *block = declarator->getType().getInterfaceBlock();
+        if (block->blockStorage() == EbsStd140)
         {
-        return (interfaceBlock->blockStorage() == EbsStd140);
+            mapBlockStructMembers(declarator->getAsSymbolNode(), block);
+        }
     }
-
     return false;
 }
 
-std::vector<TIntermTyped *> FlagStd140ValueStructs(TIntermNode *node)
+}  // anonymous namespace
+
+std::vector<MappedStruct> FlagStd140Structs(TIntermNode *node)
 {
-    FlagStd140Structs flaggingTraversal;
+    FlagStd140StructsTraverser flaggingTraversal;
 
     node->traverse(&flaggingTraversal);
 
-    return flaggingTraversal.getFlaggedNodes();
-}
+    return flaggingTraversal.getMappedStructs();
 }
+
+}  // namespace sh

src/compiler/translator/FlagStd140Structs.h

@@ -3,36 +3,28 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
+// FlagStd140Structs.h: Find structs in std140 blocks, where the padding added in the translator
+// conflicts with the "natural" unpadded type.
 
 #ifndef COMPILER_TRANSLATOR_FLAGSTD140STRUCTS_H_
 #define COMPILER_TRANSLATOR_FLAGSTD140STRUCTS_H_
 
-#include "compiler/translator/IntermTraverse.h"
+#include <vector>
 
 namespace sh
 {
 
-// This class finds references to nested structs of std140 blocks that access
-// the nested struct "by value", where the padding added in the translator
-// conflicts with the "natural" unpadded type.
-class FlagStd140Structs : public TIntermTraverser
-{
-  public:
-    FlagStd140Structs() : TIntermTraverser(true, false, false) {}
-
-    const std::vector<TIntermTyped *> getFlaggedNodes() const { return mFlaggedNodes; }
+class TField;
+class TIntermNode;
+class TIntermSymbol;
 
-  protected:
-    bool visitBinary(Visit visit, TIntermBinary *binaryNode) override;
-    void visitSymbol(TIntermSymbol *symbol) override;
-
-  private:
-    bool isInStd140InterfaceBlock(TIntermTyped *node) const;
-
-    std::vector<TIntermTyped *> mFlaggedNodes;
+struct MappedStruct
+{
+    TIntermSymbol *blockDeclarator;
+    TField *field;
 };
 
-std::vector<TIntermTyped *> FlagStd140ValueStructs(TIntermNode *node);
+std::vector<MappedStruct> FlagStd140Structs(TIntermNode *node);
 }
 
 #endif  // COMPILER_TRANSLATOR_FLAGSTD140STRUCTS_H_

src/compiler/translator/OutputHLSL.h

@@ -14,6 +14,7 @@
 #include "angle_gl.h"
 #include "compiler/translator/ASTMetadataHLSL.h"
 #include "compiler/translator/Compiler.h"
+#include "compiler/translator/FlagStd140Structs.h"
 #include "compiler/translator/IntermTraverse.h"
 
 class BuiltInFunctionEmulator;
@@ -61,7 +62,9 @@ class OutputHLSL : public TIntermTraverser
     static bool canWriteAsHLSLLiteral(TIntermTyped *expression);
 
   protected:
-    void header(TInfoSinkBase &out, const BuiltInFunctionEmulator *builtInFunctionEmulator);
+    void header(TInfoSinkBase &out,
+                const std::vector<MappedStruct> &std140Structs,
+                const BuiltInFunctionEmulator *builtInFunctionEmulator) const;
 
     void writeFloat(TInfoSinkBase &out, float f);
     void writeSingleConstant(TInfoSinkBase &out, const TConstantUnion *const constUnion);
@@ -115,7 +118,6 @@ class OutputHLSL : public TIntermTraverser
     void outputAssign(Visit visit, const TType &type, TInfoSinkBase &out);
 
     void writeEmulatedFunctionTriplet(TInfoSinkBase &out, Visit visit, TOperator op);
-    void makeFlaggedStructMaps(const std::vector<TIntermTyped *> &flaggedStructs);
 
     // Returns true if it found a 'same symbol' initializer (initializer that references the
     // variable it's initting)
@@ -204,10 +206,7 @@ class OutputHLSL : public TIntermTraverser
 
     TIntermSymbol *mExcessiveLoopIndex;
 
-    TString structInitializerString(int indent, const TType &type, const TString &name);
-
-    std::map<TIntermTyped *, TString> mFlaggedStructMappedNames;
-    std::map<TIntermTyped *, TString> mFlaggedStructOriginalNames;
+    TString structInitializerString(int indent, const TType &type, const TString &name) const;
 
     struct HelperFunction
     {
@@ -245,6 +244,7 @@ class OutputHLSL : public TIntermTraverser
     PerformanceDiagnostics *mPerfDiagnostics;
 
   private:
+    TString generateStructMapping(const std::vector<MappedStruct> &std140Structs) const;
     TString samplerNamePrefixFromStruct(TIntermTyped *node);
     bool ancestorEvaluatesToSamplerInStruct();
 };

src/tests/gl_tests/UniformBufferTest.cpp

@@ -866,31 +866,90 @@ TEST_P(UniformBufferTest, BlockContainingArrayOfStructs)
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
 }
 
+// Test with a block instance array containing an array of structs.
+TEST_P(UniformBufferTest, BlockArrayContainingArrayOfStructs)
+{
+    const std::string &fragmentShader =
+        R"(#version 300 es
+
+        precision highp float;
+        out vec4 my_FragColor;
+        struct light_t
+        {
+            vec4 intensity;
+        };
+
+        layout(std140) uniform lightData { light_t lights[2]; } buffers[2];
+
+        vec4 processLight(vec4 lighting, light_t light)
+        {
+            return lighting + light.intensity;
+        }
+        void main()
+        {
+            vec4 lighting = vec4(0, 0, 0, 1);
+            lighting = processLight(lighting, buffers[0].lights[0]);
+            lighting = processLight(lighting, buffers[1].lights[1]);
+            my_FragColor = lighting;
+        })";
+
+    ANGLE_GL_PROGRAM(program, mVertexShaderSource, fragmentShader);
+    GLint uniformBufferIndex  = glGetUniformBlockIndex(program, "lightData[0]");
+    GLint uniformBuffer2Index = glGetUniformBlockIndex(program, "lightData[1]");
+
+    glBindBuffer(GL_UNIFORM_BUFFER, mUniformBuffer);
+    const GLsizei kStructCount        = 2;
+    const GLsizei kVectorElementCount = 4;
+    const GLsizei kBytesPerElement    = 4;
+    const GLsizei kDataSize           = kStructCount * kVectorElementCount * kBytesPerElement;
+    std::vector<GLubyte> v(kDataSize, 0);
+    float *vAsFloat = reinterpret_cast<float *>(v.data());
+
+    // In the first struct/vector of the first block
+    vAsFloat[1] = 0.5f;
+
+    glBufferData(GL_UNIFORM_BUFFER, kDataSize, v.data(), GL_STATIC_DRAW);
+
+    GLBuffer uniformBuffer2;
+    glBindBuffer(GL_UNIFORM_BUFFER, uniformBuffer2);
+
+    vAsFloat[1] = 0.0f;
+    // In the second struct/vector of the second block
+    vAsFloat[kVectorElementCount + 1] = 0.5f;
+    glBufferData(GL_UNIFORM_BUFFER, kDataSize, v.data(), GL_STATIC_DRAW);
+
+    glBindBufferBase(GL_UNIFORM_BUFFER, 0, mUniformBuffer);
+    glBindBufferBase(GL_UNIFORM_BUFFER, 1, uniformBuffer2);
+    glUniformBlockBinding(program, uniformBufferIndex, 0);
+    glUniformBlockBinding(program, uniformBuffer2Index, 1);
+    drawQuad(program.get(), "position", 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
 // Test with a block containing an array of structs containing arrays.
 TEST_P(UniformBufferTest, BlockContainingArrayOfStructsContainingArrays)
 {
     const std::string &fragmentShader =
-        "#version 300 es\n"
-        "precision highp float;\n"
-        "out vec4 my_FragColor;\n"
-        "struct light_t {\n"
-        "    vec4 intensity[3];\n"
-        "};\n"
-        "const int maxLights = 2;\n"
-        "layout(std140) uniform lightData { light_t lights[maxLights]; };\n"
-        "vec4 processLight(vec4 lighting, light_t light)\n"
-        "{\n"
-        "    return lighting + light.intensity[1];\n"
-        "}\n"
-        "void main()\n"
-        "{\n"
-        "    vec4 lighting = vec4(0, 0, 0, 1);\n"
-        "    for (int n = 0; n < maxLights; n++)\n"
-        "    {\n"
-        "        lighting = processLight(lighting, lights[n]);\n"
-        "    }\n"
-        "    my_FragColor = lighting;\n"
-        "}\n";
+        R"(#version 300 es
+        precision highp float;
+        out vec4 my_FragColor;
+        struct light_t
+        {
+            vec4 intensity[3];
+        };
+        const int maxLights = 2;
+        layout(std140) uniform lightData { light_t lights[maxLights]; };
+        vec4 processLight(vec4 lighting, light_t light)
+        {
+            return lighting + light.intensity[1];
+        }
+        void main()
+        {
+            vec4 lighting = vec4(0, 0, 0, 1);
+            lighting = processLight(lighting, lights[0]);
+            lighting = processLight(lighting, lights[1]);
+            my_FragColor = lighting;
+        })";
 
     ANGLE_GL_PROGRAM(program, mVertexShaderSource, fragmentShader);
     GLint uniformBufferIndex = glGetUniformBlockIndex(program, "lightData");
@@ -1038,6 +1097,65 @@ TEST_P(UniformBufferTest, UniformBlockInstanceReservedOpenGLName)
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
 }
 
+// Test that uniform block instance with nested structs that contain vec3s inside is handled
+// correctly. This is meant to test that HLSL structure padding to implement std140 layout works
+// together with uniform blocks.
+TEST_P(UniformBufferTest, Std140UniformBlockInstanceWithNestedStructsContainingVec3s)
+{
+    // Got incorrect test result on non-NVIDIA Android - the alpha channel was not set correctly
+    // from the second vector, possibly the platform doesn't implement std140 packing right?
+    // http://anglebug.com/2217
+    ANGLE_SKIP_TEST_IF(IsAndroid() && !IsNVIDIA());
+
+    const std::string &fragmentShader =
+        R"(#version 300 es
+
+        precision highp float;
+        out vec4 my_FragColor;
+
+        struct Sinner {
+          vec3 v;
+        };
+
+        struct S {
+            Sinner s1;
+            Sinner s2;
+        };
+
+        layout(std140) uniform structBuffer { S s; } buffer;
+
+        void accessStruct(S s)
+        {
+            my_FragColor = vec4(s.s1.v.xy, s.s2.v.xy);
+        }
+
+        void main()
+        {
+            accessStruct(buffer.s);
+        })";
+
+    ANGLE_GL_PROGRAM(program, mVertexShaderSource, fragmentShader);
+    GLint uniformBufferIndex = glGetUniformBlockIndex(program, "structBuffer");
+
+    glBindBuffer(GL_UNIFORM_BUFFER, mUniformBuffer);
+    const GLsizei kVectorsPerBlock         = 2;
+    const GLsizei kElementsPerPaddedVector = 4;
+    const GLsizei kBytesPerElement         = 4;
+    const GLsizei kDataSize = kVectorsPerBlock * kElementsPerPaddedVector * kBytesPerElement;
+    std::vector<GLubyte> v(kDataSize, 0);
+    float *vAsFloat = reinterpret_cast<float *>(v.data());
+
+    // Set second value in each vec3.
+    vAsFloat[1u]      = 1.0f;
+    vAsFloat[4u + 1u] = 1.0f;
+
+    glBufferData(GL_UNIFORM_BUFFER, kDataSize, v.data(), GL_STATIC_DRAW);
+    glBindBufferBase(GL_UNIFORM_BUFFER, 0, mUniformBuffer);
+    glUniformBlockBinding(program, uniformBufferIndex, 0);
+    drawQuad(program.get(), "position", 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these tests should be run against.
 ANGLE_INSTANTIATE_TEST(UniformBufferTest,
                        ES3_D3D11(),