D3D11: Fix varying packing with structs.

src/libANGLE/Shader.cpp

@@ -51,7 +51,7 @@ const std::vector<VarT> &GetShaderVariables(const std::vector<VarT> *variableLis
 }  // anonymous namespace
 
 // true if varying x has a higher priority in packing than y
-bool CompareVarying(const sh::Varying &x, const sh::Varying &y)
+bool CompareShaderVar(const sh::ShaderVariable &x, const sh::ShaderVariable &y)
 {
     if (x.type == y.type)
     {
@@ -293,7 +293,7 @@ void Shader::compile(Compiler *compiler)
         ASSERT(mData.mShaderType == GL_FRAGMENT_SHADER);
 
         // TODO(jmadill): Figure out why we only sort in the FS, and if we need to.
-        std::sort(mData.mVaryings.begin(), mData.mVaryings.end(), CompareVarying);
+        std::sort(mData.mVaryings.begin(), mData.mVaryings.end(), CompareShaderVar);
         mData.mActiveOutputVariables =
             GetActiveShaderVariables(ShGetOutputVariables(compilerHandle));
     }

src/libANGLE/Shader.h

@@ -137,7 +137,7 @@ class Shader : angle::NonCopyable
     ResourceManager *mResourceManager;
 };
 
-bool CompareVarying(const sh::Varying &x, const sh::Varying &y);
+bool CompareShaderVar(const sh::ShaderVariable &x, const sh::ShaderVariable &y);
 }
 
 #endif   // LIBANGLE_SHADER_H_

src/libANGLE/renderer/d3d/DynamicHLSL.cpp

@@ -18,10 +18,6 @@
 #include "libANGLE/renderer/d3d/ShaderD3D.h"
 #include "libANGLE/renderer/d3d/VaryingPacking.h"
 
-// For use with ArrayString, see angleutils.h
-static_assert(GL_INVALID_INDEX == UINT_MAX,
-              "GL_INVALID_INDEX must be equal to the max unsigned int.");
-
 using namespace gl;
 
 namespace rx
@@ -107,6 +103,20 @@ const PixelShaderOutputVariable *FindOutputAtLocation(
     return nullptr;
 }
 
+void WriteArrayString(std::stringstream &strstr, unsigned int i)
+{
+    static_assert(GL_INVALID_INDEX == UINT_MAX,
+                  "GL_INVALID_INDEX must be equal to the max unsigned int.");
+    if (i == UINT_MAX)
+    {
+        return;
+    }
+
+    strstr << "[";
+    strstr << i;
+    strstr << "]";
+}
+
 const std::string VERTEX_ATTRIBUTE_STUB_STRING = "@@ VERTEX ATTRIBUTES @@";
 const std::string PIXEL_OUTPUT_STUB_STRING     = "@@ PIXEL OUTPUT @@";
 }  // anonymous namespace
@@ -132,9 +142,8 @@ void DynamicHLSL::generateVaryingHLSL(const VaryingPacking &varyingPacking,
 
     for (const PackedVaryingRegister &registerInfo : varyingPacking.getRegisterList())
     {
-        const sh::Varying &varying = *registerInfo.packedVarying->varying;
-        GLenum transposedType      = gl::TransposeMatrixType(varying.type);
-        unsigned int semanticIndex = registerInfo.semanticIndex;
+        const auto &varying = *registerInfo.packedVarying->varying;
+        ASSERT(!varying.isStruct());
 
         // TODO: Add checks to ensure D3D interpolation modifiers don't result in too many
         // registers being used.
@@ -143,7 +152,7 @@ void DynamicHLSL::generateVaryingHLSL(const VaryingPacking &varyingPacking,
         // If the float varying has the 'nointerpolation' modifier on it then we would need
         // N + 1 registers, and D3D compilation will fail.
 
-        switch (varying.interpolation)
+        switch (registerInfo.packedVarying->interpolation)
         {
             case sh::INTERPOLATION_SMOOTH:
                 hlslStream << "    ";
@@ -158,18 +167,11 @@ void DynamicHLSL::generateVaryingHLSL(const VaryingPacking &varyingPacking,
                 UNREACHABLE();
         }
 
-        if (varying.isStruct())
-        {
-            // TODO(jmadill): pass back translated name from the shader translator
-            hlslStream << decorateVariable(varying.structName);
-        }
-        else
-        {
-            GLenum componentType = VariableComponentType(transposedType);
-            int columnCount = VariableColumnCount(transposedType);
-            hlslStream << HLSLComponentTypeString(componentType, columnCount);
-        }
-
+        GLenum transposedType = gl::TransposeMatrixType(varying.type);
+        GLenum componentType  = gl::VariableComponentType(transposedType);
+        int columnCount = gl::VariableColumnCount(transposedType);
+        hlslStream << HLSLComponentTypeString(componentType, columnCount);
+        unsigned int semanticIndex = registerInfo.semanticIndex;
         hlslStream << " v" << semanticIndex << " : " << varyingSemantic << semanticIndex << ";\n";
     }
 }
@@ -490,21 +492,27 @@ bool DynamicHLSL::generateShaderLinkHLSL(const gl::Data &data,
 
     for (const PackedVaryingRegister &registerInfo : varyingPacking.getRegisterList())
     {
-        const sh::Varying &varying = *registerInfo.packedVarying->varying;
-        GLenum transposedType = TransposeMatrixType(varying.type);
-        unsigned int variableRows =
-            static_cast<unsigned int>(varying.isStruct() ? 1 : VariableRowCount(transposedType));
+        const auto &packedVarying = *registerInfo.packedVarying;
+        const auto &varying = *packedVarying.varying;
+        ASSERT(!varying.isStruct());
+
+        vertexStream << "    output.v" << registerInfo.semanticIndex << " = ";
 
-        vertexStream << "    output.v" << registerInfo.semanticIndex << " = _" + varying.name;
+        if (packedVarying.isStructField())
+        {
+            vertexStream << decorateVariable(packedVarying.parentStructName) << ".";
+        }
+
+        vertexStream << decorateVariable(varying.name);
 
         if (varying.isArray())
         {
-            vertexStream << ArrayString(registerInfo.varyingArrayIndex);
+            WriteArrayString(vertexStream, registerInfo.varyingArrayIndex);
         }
 
-        if (variableRows > 1)
+        if (VariableRowCount(varying.type) > 1)
         {
-            vertexStream << ArrayString(registerInfo.varyingRowIndex);
+            WriteArrayString(vertexStream, registerInfo.varyingRowIndex);
         }
 
         vertexStream << ";\n";
@@ -623,47 +631,51 @@ bool DynamicHLSL::generateShaderLinkHLSL(const gl::Data &data,
 
     for (const PackedVaryingRegister &registerInfo : varyingPacking.getRegisterList())
     {
-        const sh::Varying &varying = *registerInfo.packedVarying->varying;
+        const auto &packedVarying = *registerInfo.packedVarying;
+        const auto &varying = *packedVarying.varying;
+        ASSERT(!varying.isBuiltIn() && !varying.isStruct());
 
         // Don't reference VS-only transform feedback varyings in the PS.
         if (registerInfo.packedVarying->vertexOnly)
             continue;
 
-        ASSERT(!varying.isBuiltIn());
-        GLenum transposedType      = TransposeMatrixType(varying.type);
-        int variableRows           = (varying.isStruct() ? 1 : VariableRowCount(transposedType));
-        pixelStream << "    _" << varying.name;
+        pixelStream << "    ";
+
+        if (packedVarying.isStructField())
+        {
+            pixelStream << decorateVariable(packedVarying.parentStructName) << ".";
+        }
+
+        pixelStream << decorateVariable(varying.name);
 
         if (varying.isArray())
         {
-            pixelStream << ArrayString(registerInfo.varyingArrayIndex);
+            WriteArrayString(pixelStream, registerInfo.varyingArrayIndex);
         }
 
-        if (variableRows > 1)
+        GLenum transposedType = TransposeMatrixType(varying.type);
+        if (VariableRowCount(transposedType) > 1)
         {
-            pixelStream << ArrayString(registerInfo.varyingRowIndex);
+            WriteArrayString(pixelStream, registerInfo.varyingRowIndex);
         }
 
         pixelStream << " = input.v" << registerInfo.semanticIndex;
 
-        if (!varying.isStruct())
+        switch (VariableColumnCount(transposedType))
         {
-            switch (VariableColumnCount(transposedType))
-            {
-                case 1:
-                    pixelStream << ".x";
-                    break;
-                case 2:
-                    pixelStream << ".xy";
-                    break;
-                case 3:
-                    pixelStream << ".xyz";
-                    break;
-                case 4:
-                    break;
-                default:
-                    UNREACHABLE();
-            }
+            case 1:
+                pixelStream << ".x";
+                break;
+            case 2:
+                pixelStream << ".xy";
+                break;
+            case 3:
+                pixelStream << ".xyz";
+                break;
+            case 4:
+                break;
+            default:
+                UNREACHABLE();
         }
         pixelStream << ";\n";
     }
@@ -712,10 +724,8 @@ std::string DynamicHLSL::generateGeometryShaderPreamble(const VaryingPacking &va
 
     for (const PackedVaryingRegister &varyingRegister : varyingPacking.getRegisterList())
     {
-        const sh::Varying &varying = *varyingRegister.packedVarying->varying;
-
         preambleStream << "    output.v" << varyingRegister.semanticIndex << " = ";
-        if (varying.interpolation == sh::INTERPOLATION_FLAT)
+        if (varyingRegister.packedVarying->interpolation == sh::INTERPOLATION_FLAT)
         {
             preambleStream << "flat";
         }

src/libANGLE/renderer/d3d/ProgramD3D.cpp

@@ -104,7 +104,7 @@ struct AttributeSorter
 // true if varying x has a higher priority in packing than y
 bool ComparePackedVarying(const PackedVarying &x, const PackedVarying &y)
 {
-    return gl::CompareVarying(*x.varying, *y.varying);
+    return gl::CompareShaderVar(*x.varying, *y.varying);
 }
 
 std::vector<PackedVarying> MergeVaryings(const gl::Shader &vertexShader,
@@ -127,7 +127,20 @@ std::vector<PackedVarying> MergeVaryings(const gl::Shader &vertexShader,
         {
             if (output.name == input.name)
             {
-                packedVaryings.push_back(PackedVarying(input));
+                if (output.isStruct())
+                {
+                    ASSERT(!output.isArray());
+                    for (const auto &field : output.fields)
+                    {
+                        ASSERT(!field.isStruct() && !field.isArray());
+                        packedVaryings.push_back(
+                            PackedVarying(field, input.interpolation, input.name));
+                    }
+                }
+                else
+                {
+                    packedVaryings.push_back(PackedVarying(input, input.interpolation));
+                }
                 packed = true;
                 break;
             }
@@ -140,8 +153,14 @@ std::vector<PackedVarying> MergeVaryings(const gl::Shader &vertexShader,
             {
                 if (tfVarying == output.name)
                 {
-                    packedVaryings.push_back(PackedVarying(output));
-                    packedVaryings.back().vertexOnly = true;
+                    // Transform feedback for varying structs is underspecified.
+                    // See Khronos bug 9856.
+                    // TODO(jmadill): Figure out how to be spec-compliant here.
+                    if (!output.isStruct())
+                    {
+                        packedVaryings.push_back(PackedVarying(output, output.interpolation));
+                        packedVaryings.back().vertexOnly = true;
+                    }
                     break;
                 }
             }
@@ -1391,9 +1410,10 @@ LinkResult ProgramD3D::link(const gl::Data &data, gl::InfoLog &infoLog)
     mUsesFragDepth = metadata.usesFragDepth(mData);
 
     // Cache if we use flat shading
+    mUsesFlatInterpolation = false;
     for (const auto &varying : packedVaryings)
     {
-        if (varying.varying->interpolation == sh::INTERPOLATION_FLAT)
+        if (varying.interpolation == sh::INTERPOLATION_FLAT)
         {
             mUsesFlatInterpolation = true;
             break;
@@ -2251,11 +2271,17 @@ void ProgramD3D::gatherTransformFeedbackVaryings(const VaryingPacking &varyingPa
         {
             for (const PackedVaryingRegister &registerInfo : varyingPacking.getRegisterList())
             {
-                const sh::Varying &varying = *registerInfo.packedVarying->varying;
-                GLenum transposedType      = gl::TransposeMatrixType(varying.type);
+                const auto &varying   = *registerInfo.packedVarying->varying;
+                GLenum transposedType = gl::TransposeMatrixType(varying.type);
                 int componentCount = gl::VariableColumnCount(transposedType);
                 ASSERT(!varying.isBuiltIn());
 
+                // Transform feedback for varying structs is underspecified.
+                // See Khronos bug 9856.
+                // TODO(jmadill): Figure out how to be spec-compliant here.
+                if (registerInfo.packedVarying->isStructField() || varying.isStruct())
+                    continue;
+
                 // There can be more than one register assigned to a particular varying, and each
                 // register needs its own stream out entry.
                 if (tfVaryingName == varying.name)

src/libANGLE/renderer/d3d/VaryingPacking.cpp

@@ -57,23 +57,16 @@ bool VaryingPacking::packVarying(const PackedVarying &packedVarying)
     unsigned int varyingRows    = 0;
     unsigned int varyingColumns = 0;
 
-    const sh::Varying &varying = *packedVarying.varying;
+    const auto &varying = *packedVarying.varying;
 
-    if (varying.isStruct())
-    {
-        varyingRows    = HLSLVariableRegisterCount(varying, true);
-        varyingColumns = 4;
-    }
-    else
-    {
-        // "Non - square matrices of type matCxR consume the same space as a square matrix of type
-        // matN where N is the greater of C and R.Variables of type mat2 occupies 2 complete rows."
-        // Here we are a bit more conservative and allow packing non-square matrices more tightly.
-        // Make sure we use transposed matrix types to count registers correctly.
-        GLenum transposedType = gl::TransposeMatrixType(varying.type);
-        varyingRows           = gl::VariableRowCount(transposedType);
-        varyingColumns        = gl::VariableColumnCount(transposedType);
-    }
+    // "Non - square matrices of type matCxR consume the same space as a square matrix of type matN
+    // where N is the greater of C and R.Variables of type mat2 occupies 2 complete rows."
+    // Here we are a bit more conservative and allow packing non-square matrices more tightly.
+    // Make sure we use transposed matrix types to count registers correctly.
+    ASSERT(!varying.isStruct());
+    GLenum transposedType = gl::TransposeMatrixType(varying.type);
+    varyingRows           = gl::VariableRowCount(transposedType);
+    varyingColumns        = gl::VariableColumnCount(transposedType);
 
     // "Arrays of size N are assumed to take N times the size of the base type"
     varyingRows *= varying.elementCount();
@@ -206,18 +199,11 @@ void VaryingPacking::insert(unsigned int registerRow,
     unsigned int varyingRows    = 0;
     unsigned int varyingColumns = 0;
 
-    const sh::Varying &varying = *packedVarying.varying;
-    if (varying.isStruct())
-    {
-        varyingRows    = HLSLVariableRegisterCount(varying, true);
-        varyingColumns = 4;
-    }
-    else
-    {
-        GLenum transposedType = gl::TransposeMatrixType(varying.type);
-        varyingRows           = gl::VariableRowCount(transposedType);
-        varyingColumns        = gl::VariableColumnCount(transposedType);
-    }
+    const auto &varying = *packedVarying.varying;
+    ASSERT(!varying.isStruct());
+    GLenum transposedType = gl::TransposeMatrixType(varying.type);
+    varyingRows           = gl::VariableRowCount(transposedType);
+    varyingColumns        = gl::VariableColumnCount(transposedType);
 
     PackedVaryingRegister registerInfo;
     registerInfo.packedVarying  = &packedVarying;
@@ -251,14 +237,15 @@ bool VaryingPacking::packVaryings(gl::InfoLog &infoLog,
     // subrectangle. No splitting of variables is permitted."
     for (const PackedVarying &packedVarying : packedVaryings)
     {
-        const sh::Varying &varying = *packedVarying.varying;
+        const auto &varying = *packedVarying.varying;
 
         // Do not assign registers to built-in or unreferenced varyings
-        if (varying.isBuiltIn() || !varying.staticUse)
+        if (varying.isBuiltIn() || (!varying.staticUse && !packedVarying.isStructField()))
         {
             continue;
         }
 
+        ASSERT(!varying.isStruct());
         ASSERT(uniqueVaryingNames.count(varying.name) == 0);
 
         if (packVarying(packedVarying))
@@ -282,7 +269,7 @@ bool VaryingPacking::packVaryings(gl::InfoLog &infoLog,
 
         for (const PackedVarying &packedVarying : packedVaryings)
         {
-            const sh::Varying &varying = *packedVarying.varying;
+            const auto &varying = *packedVarying.varying;
 
             // Make sure transform feedback varyings aren't optimized out.
             if (uniqueVaryingNames.count(transformFeedbackVaryingName) == 0)

src/libANGLE/renderer/d3d/VaryingPacking.h

@@ -19,12 +19,32 @@ class ProgramD3DMetadata;
 
 struct PackedVarying
 {
-    PackedVarying(const sh::Varying &varyingIn) : varying(&varyingIn), vertexOnly(false) {}
+    PackedVarying(const sh::ShaderVariable &varyingIn, sh::InterpolationType interpolationIn)
+        : varying(&varyingIn), vertexOnly(false), interpolation(interpolationIn)
+    {
+    }
+    PackedVarying(const sh::ShaderVariable &varyingIn,
+                  sh::InterpolationType interpolationIn,
+                  const std::string &parentStructNameIn)
+        : varying(&varyingIn),
+          vertexOnly(false),
+          interpolation(interpolationIn),
+          parentStructName(parentStructNameIn)
+    {
+    }
+
+    bool isStructField() const { return !parentStructName.empty(); }
 
-    const sh::Varying *varying;
+    const sh::ShaderVariable *varying;
 
     // Transform feedback varyings can be only referenced in the VS.
     bool vertexOnly;
+
+    // Cached so we can store sh::ShaderVariable to point to varying fields.
+    sh::InterpolationType interpolation;
+
+    // Struct name
+    std::string parentStructName;
 };
 
 struct PackedVaryingRegister final
@@ -52,6 +72,8 @@ struct PackedVaryingRegister final
         return registerRow * 4 + registerColumn;
     }
 
+    bool isStructField() const { return !structFieldName.empty(); }
+
     // Index to the array of varyings.
     const PackedVarying *packedVarying;
 
@@ -69,6 +91,9 @@ struct PackedVaryingRegister final
 
     // Assigned after packing
     unsigned int semanticIndex;
+
+    // Struct member this varying corresponds to.
+    std::string structFieldName;
 };
 
 class VaryingPacking final : angle::NonCopyable

src/tests/deqp_support/deqp_gles3_test_expectations.txt

@@ -40,8 +40,6 @@
 
 // TODO(jmadill): triage these into permanent and temporary failures
 
-1088 WIN : dEQP-GLES3.functional.shaders.linkage.varying.struct.float_vec3 = FAIL
-1088 WIN : dEQP-GLES3.functional.shaders.linkage.varying.struct.float_uvec2_vec3 = FAIL
 1089 WIN : dEQP-GLES3.functional.shaders.functions.invalid.local_function_prototype_vertex = FAIL
 1089 WIN : dEQP-GLES3.functional.shaders.functions.invalid.local_function_prototype_fragment = FAIL
 1089 WIN : dEQP-GLES3.functional.shaders.functions.invalid.overload_builtin_function_vertex = FAIL