D3D9: Improve varying packing failure mode.

src/libANGLE/Caps.cpp

@@ -253,7 +253,8 @@ Limitations::Limitations()
       attributeZeroRequiresZeroDivisorInEXT(false),
       noSeparateStencilRefsAndMasks(false),
       shadersRequireIndexedLoopValidation(false),
-      noSimultaneousConstantColorAndAlphaBlendFunc(false)
+      noSimultaneousConstantColorAndAlphaBlendFunc(false),
+      noFlexibleVaryingPacking(false)
 {
 }
 

src/libANGLE/Caps.h

@@ -410,6 +410,9 @@ struct Limitations
     // Renderer doesn't support Simultaneous use of GL_CONSTANT_ALPHA/GL_ONE_MINUS_CONSTANT_ALPHA
     // and GL_CONSTANT_COLOR/GL_ONE_MINUS_CONSTANT_COLOR blend functions.
     bool noSimultaneousConstantColorAndAlphaBlendFunc;
+
+    // D3D9 does not support flexible varying register packing.
+    bool noFlexibleVaryingPacking;
 };
 
 struct TypePrecision

src/libANGLE/Program.cpp

@@ -986,8 +986,16 @@ Error Program::link(const gl::Context *context)
 
         // Map the varyings to the register file
         // In WebGL, we use a slightly different handling for packing variables.
-        auto packMode = data.getExtensions().webglCompatibility ? PackMode::WEBGL_STRICT
-                                                                : PackMode::ANGLE_RELAXED;
+        gl::PackMode packMode = PackMode::ANGLE_RELAXED;
+        if (data.getLimitations().noFlexibleVaryingPacking)
+        {
+            // D3D9 pack mode is strictly more strict than WebGL, so takes priority.
+            packMode = PackMode::ANGLE_NON_CONFORMANT_D3D9;
+        }
+        else if (data.getExtensions().webglCompatibility)
+        {
+            packMode = PackMode::WEBGL_STRICT;
+        }
 
         ProgramLinkedResources resources = {
             {data.getCaps().maxVaryingVectors, packMode},

src/libANGLE/VaryingPacking.cpp

@@ -69,7 +69,7 @@ VaryingPacking::~VaryingPacking() = default;
 // Returns false if unsuccessful.
 bool VaryingPacking::packVarying(const PackedVarying &packedVarying)
 {
-    const auto &varying = *packedVarying.varying;
+    const sh::ShaderVariable &varying = *packedVarying.varying;
 
     // "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."
@@ -80,9 +80,16 @@ bool VaryingPacking::packVarying(const PackedVarying &packedVarying)
     unsigned int varyingRows    = gl::VariableRowCount(transposedType);
     unsigned int varyingColumns = gl::VariableColumnCount(transposedType);
 
+    // Special pack mode for D3D9. Each varying takes a full register, no sharing.
+    // TODO(jmadill): Implement more sophisticated component packing in D3D9.
+    if (mPackMode == PackMode::ANGLE_NON_CONFORMANT_D3D9)
+    {
+        varyingColumns = 4;
+    }
+
     // "Variables of type mat2 occupies 2 complete rows."
     // For non-WebGL contexts, we allow mat2 to occupy only two columns per row.
-    if (mPackMode == PackMode::WEBGL_STRICT && varying.type == GL_FLOAT_MAT2)
+    else if (mPackMode == PackMode::WEBGL_STRICT && varying.type == GL_FLOAT_MAT2)
     {
         varyingColumns = 4;
     }
@@ -286,29 +293,40 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
 
         // Only pack statically used varyings that have a matched input or output, plus special
         // builtins.
-        if (((input && output) || (output && output->isBuiltIn())) && output->staticUse)
+        if ((input && output && output->staticUse) ||
+            (input && input->isBuiltIn() && input->staticUse) ||
+            (output && output->isBuiltIn() && output->staticUse))
         {
-            // Will get the vertex shader interpolation by default.
-            auto interpolation = ref.second.get()->interpolation;
+            const sh::Varying *varying = output ? output : input;
 
-            // Note that we lose the vertex shader static use information here. The data for the
-            // variable is taken from the fragment shader.
-            if (output->isStruct())
+            // Don't count gl_Position. Also don't count gl_PointSize for D3D9.
+            if (varying->name != "gl_Position" &&
+                !(varying->name == "gl_PointSize" &&
+                  mPackMode == PackMode::ANGLE_NON_CONFORMANT_D3D9))
             {
-                ASSERT(!output->isArray());
-                for (const auto &field : output->fields)
+                // Will get the vertex shader interpolation by default.
+                auto interpolation = ref.second.get()->interpolation;
+
+                // Note that we lose the vertex shader static use information here. The data for the
+                // variable is taken from the fragment shader.
+                if (varying->isStruct())
+                {
+                    ASSERT(!varying->isArray());
+                    for (const auto &field : varying->fields)
+                    {
+                        ASSERT(!field.isStruct() && !field.isArray());
+                        mPackedVaryings.push_back(
+                            PackedVarying(field, interpolation, varying->name));
+                        uniqueFullNames.insert(mPackedVaryings.back().fullName());
+                    }
+                }
+                else
                 {
-                    ASSERT(!field.isStruct() && !field.isArray());
-                    mPackedVaryings.push_back(PackedVarying(field, interpolation, output->name));
+                    mPackedVaryings.push_back(PackedVarying(*varying, interpolation));
                     uniqueFullNames.insert(mPackedVaryings.back().fullName());
                 }
+                continue;
             }
-            else
-            {
-                mPackedVaryings.push_back(PackedVarying(*output, interpolation));
-                uniqueFullNames.insert(mPackedVaryings.back().fullName());
-            }
-            continue;
         }
 
         // Keep Transform FB varyings in the merged list always.
@@ -383,6 +401,14 @@ bool VaryingPacking::packUserVaryings(gl::InfoLog &infoLog,
         if (!packVarying(packedVarying))
         {
             infoLog << "Could not pack varying " << packedVarying.fullName();
+
+            // TODO(jmadill): Implement more sophisticated component packing in D3D9.
+            if (mPackMode == PackMode::ANGLE_NON_CONFORMANT_D3D9)
+            {
+                infoLog << "Note: Additional non-conformant packing restrictions are enforced on "
+                           "D3D9.";
+            }
+
             return false;
         }
     }
@@ -400,19 +426,4 @@ bool VaryingPacking::packUserVaryings(gl::InfoLog &infoLog,
     return true;
 }
 
-unsigned int VaryingPacking::getRegisterCount() const
-{
-    unsigned int count = 0;
-
-    for (const Register &reg : mRegisterMap)
-    {
-        if (reg.data[0] || reg.data[1] || reg.data[2] || reg.data[3])
-        {
-            ++count;
-        }
-    }
-
-    return count;
-}
-
 }  // namespace rx

src/libANGLE/VaryingPacking.h

@@ -141,6 +141,9 @@ enum class PackMode
 
     // We allow mat2 to take a 2x2 chunk.
     ANGLE_RELAXED,
+
+    // Each varying takes a separate register. No register sharing.
+    ANGLE_NON_CONFORMANT_D3D9,
 };
 
 class VaryingPacking final : angle::NonCopyable
@@ -175,8 +178,6 @@ class VaryingPacking final : angle::NonCopyable
     {
         return static_cast<unsigned int>(mRegisterList.size());
     }
-    unsigned int getRegisterCount() const;
-    size_t getRegisterMapSize() const { return mRegisterMap.size(); }
 
   private:
     bool packVarying(const PackedVarying &packedVarying);

src/libANGLE/renderer/d3d/ProgramD3D.cpp

@@ -1729,16 +1729,6 @@ gl::LinkResult ProgramD3D::link(const gl::Context *context,
             }
         }
 
-        // TODO(jmadill): Implement more sophisticated component packing in D3D9.
-        // We can fail here because we use one semantic per GLSL varying. D3D11 can pack varyings
-        // intelligently, but D3D9 assumes one semantic per register.
-        if (mRenderer->getRendererClass() == RENDERER_D3D9 &&
-            resources.varyingPacking.getMaxSemanticIndex() > data.getCaps().maxVaryingVectors)
-        {
-            infoLog << "Cannot pack these varyings on D3D9.";
-            return false;
-        }
-
         ProgramD3DMetadata metadata(mRenderer, vertexShaderD3D, fragmentShaderD3D);
         BuiltinVaryingsD3D builtins(metadata, resources.varyingPacking);
 

src/libANGLE/renderer/d3d/d3d9/renderer9_utils.cpp

@@ -292,6 +292,15 @@ D3DMULTISAMPLE_TYPE GetMultisampleType(GLuint samples)
 namespace d3d9_gl
 {
 
+unsigned int GetReservedVaryingVectors()
+{
+    // We reserve two registers for "dx_Position" and "gl_Position". The spec says they
+    // don't count towards the varying limit, so we must make space for them. We also
+    // reserve the last register since it can only pass a PSIZE, and not any arbitrary
+    // varying.
+    return 3;
+}
+
 unsigned int GetReservedVertexUniformVectors()
 {
     return 3;  // dx_ViewCoords, dx_ViewAdjust and dx_DepthRange.
@@ -472,9 +481,9 @@ void GenerateCaps(IDirect3D9 *d3d9,
 
     caps->maxVertexUniformBlocks = 0;
 
-    // SM3 only supports 11 output variables, with a special 12th register for PSIZE.
-    const size_t MAX_VERTEX_OUTPUT_VECTORS_SM3 = 9;
-    const size_t MAX_VERTEX_OUTPUT_VECTORS_SM2 = 7;
+    // SM3 only supports 12 output variables, but the special 12th register is only for PSIZE.
+    const unsigned int MAX_VERTEX_OUTPUT_VECTORS_SM3 = 12 - GetReservedVaryingVectors();
+    const unsigned int MAX_VERTEX_OUTPUT_VECTORS_SM2 = 10 - GetReservedVaryingVectors();
     caps->maxVertexOutputComponents = ((deviceCaps.VertexShaderVersion >= D3DVS_VERSION(3, 0)) ? MAX_VERTEX_OUTPUT_VECTORS_SM3
                                                                                                : MAX_VERTEX_OUTPUT_VECTORS_SM2) * 4;
 
@@ -615,6 +624,10 @@ void GenerateCaps(IDirect3D9 *d3d9,
 
     // D3D9 cannot support constant color and alpha blend funcs together
     limitations->noSimultaneousConstantColorAndAlphaBlendFunc = true;
+
+    // D3D9 cannot support packing more than one variable to a single varying.
+    // TODO(jmadill): Implement more sophisticated component packing in D3D9.
+    limitations->noFlexibleVaryingPacking = true;
 }
 
 }

src/libANGLE/renderer/d3d/d3d9/renderer9_utils.h

@@ -48,6 +48,8 @@ D3DMULTISAMPLE_TYPE GetMultisampleType(GLuint samples);
 namespace d3d9_gl
 {
 
+unsigned int GetReservedVaryingVectors();
+
 unsigned int GetReservedVertexUniformVectors();
 
 unsigned int GetReservedFragmentUniformVectors();

src/tests/gl_tests/GLSLTest.cpp

@@ -1297,29 +1297,62 @@ TEST_P(GLSLTest, MaxVaryingVec4)
     VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings, 0, false, false, false, true);
 }
 
-TEST_P(GLSLTest, MaxMinusTwoVaryingVec4PlusTwoSpecialVariables)
+// Verify we can pack registers with one builtin varying.
+TEST_P(GLSLTest, MaxVaryingVec4_OneBuiltin)
 {
     GLint maxVaryings = 0;
     glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
 
-    // Generate shader code that uses gl_FragCoord and gl_PointCoord, two special fragment shader variables.
-    VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings - 2, 0, true, true, false, true);
+    // Generate shader code that uses gl_FragCoord.
+    VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings - 1, 0, true, false, false, true);
 }
 
-TEST_P(GLSLTest, MaxMinusTwoVaryingVec4PlusThreeSpecialVariables)
+// Verify we can pack registers with two builtin varyings.
+TEST_P(GLSLTest, MaxVaryingVec4_TwoBuiltins)
 {
-    // TODO(geofflang): Figure out why this fails on OpenGL AMD (http://anglebug.com/1291)
-    if (IsAMD() && getPlatformRenderer() == EGL_PLATFORM_ANGLE_TYPE_OPENGL_ANGLE)
-    {
-        std::cout << "Test disabled on OpenGL." << std::endl;
-        return;
-    }
+    GLint maxVaryings = 0;
+    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
+
+    // Generate shader code that uses gl_FragCoord and gl_PointCoord.
+    VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings - 2, 0, true, true, false, true);
+}
 
+// Verify we can pack registers with three builtin varyings.
+TEST_P(GLSLTest, MaxVaryingVec4_ThreeBuiltins)
+{
     GLint maxVaryings = 0;
     glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
 
     // Generate shader code that uses gl_FragCoord, gl_PointCoord and gl_PointSize.
-    VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings - 2, 0, true, true, true, true);
+    VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings - 3, 0, true, true, true, true);
+}
+
+// This covers a problematic case in D3D9 - we are limited by the number of available semantics,
+// rather than total register use.
+TEST_P(GLSLTest, MaxVaryingsSpecialCases)
+{
+    ANGLE_SKIP_TEST_IF(!IsD3D9());
+
+    GLint maxVaryings = 0;
+    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
+
+    VaryingTestBase(maxVaryings, 0, 0, 0, 0, 0, 0, 0, true, false, false, false);
+    VaryingTestBase(maxVaryings - 1, 0, 0, 0, 0, 0, 0, 0, true, true, false, false);
+    VaryingTestBase(maxVaryings - 2, 0, 0, 0, 0, 0, 0, 0, true, true, false, true);
+
+    // Special case for gl_PointSize: we get it for free on D3D9.
+    VaryingTestBase(maxVaryings - 2, 0, 0, 0, 0, 0, 0, 0, true, true, true, true);
+}
+
+// This covers a problematic case in D3D9 - we are limited by the number of available semantics,
+// rather than total register use.
+TEST_P(GLSLTest, MaxMinusTwoVaryingVec2PlusOneSpecialVariable)
+{
+    GLint maxVaryings = 0;
+    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
+
+    // Generate shader code that uses gl_FragCoord.
+    VaryingTestBase(0, 0, maxVaryings, 0, 0, 0, 0, 0, true, false, false, !IsD3D9());
 }
 
 TEST_P(GLSLTest, MaxVaryingVec3)
@@ -1338,45 +1371,27 @@ TEST_P(GLSLTest, MaxVaryingVec3Array)
     VaryingTestBase(0, 0, 0, 0, 0, maxVaryings / 2, 0, 0, false, false, false, true);
 }
 
-// Disabled because of a failure in D3D9
+// Only fails on D3D9 because of packing limitations.
 TEST_P(GLSLTest, MaxVaryingVec3AndOneFloat)
 {
-    if (IsD3D9())
-    {
-        std::cout << "Test disabled on D3D9." << std::endl;
-        return;
-    }
-
     GLint maxVaryings = 0;
     glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
 
-    VaryingTestBase(1, 0, 0, 0, maxVaryings, 0, 0, 0, false, false, false, true);
+    VaryingTestBase(1, 0, 0, 0, maxVaryings, 0, 0, 0, false, false, false, !IsD3D9());
 }
 
-// Disabled because of a failure in D3D9
+// Only fails on D3D9 because of packing limitations.
 TEST_P(GLSLTest, MaxVaryingVec3ArrayAndOneFloatArray)
 {
-    if (IsD3D9())
-    {
-        std::cout << "Test disabled on D3D9." << std::endl;
-        return;
-    }
-
     GLint maxVaryings = 0;
     glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
 
-    VaryingTestBase(0, 1, 0, 0, 0, maxVaryings / 2, 0, 0, false, false, false, true);
+    VaryingTestBase(0, 1, 0, 0, 0, maxVaryings / 2, 0, 0, false, false, false, !IsD3D9());
 }
 
-// Disabled because of a failure in D3D9
+// Only fails on D3D9 because of packing limitations.
 TEST_P(GLSLTest, TwiceMaxVaryingVec2)
 {
-    if (IsD3D9())
-    {
-        std::cout << "Test disabled on D3D9." << std::endl;
-        return;
-    }
-
     if (getPlatformRenderer() == EGL_PLATFORM_ANGLE_TYPE_OPENGLES_ANGLE)
     {
         // TODO(geofflang): Figure out why this fails on NVIDIA's GLES driver
@@ -1397,7 +1412,7 @@ TEST_P(GLSLTest, TwiceMaxVaryingVec2)
     GLint maxVaryings = 0;
     glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
 
-    VaryingTestBase(0, 0, 2 * maxVaryings, 0, 0, 0, 0, 0, false, false, false, true);
+    VaryingTestBase(0, 0, 2 * maxVaryings, 0, 0, 0, 0, 0, false, false, false, !IsD3D9());
 }
 
 // Disabled because of a failure in D3D9