D3D9: Improve varying packing failure mode.
D3D9 has a special limitation on varying packing, where each
variable takes up a full register width, and cannot share space
with other packed varyings.
A bug was counting registers incorrectly on D3D9. Fix this by
introducing a new limitation exposed to the ANGLE front-end via
the gl::Limitations structure. Now varying packing will fail
correctly in the ANGLE linking front-end with a more descriptive
error message, as such:
"Could not pack varying blah"
"Note: Additional non-conformant packing restrictions are enforced on D3D9."
Also change the packing so that input built-in variables are
counted towards varying limits (e.g. gl_PointSize), except for
gl_Position. On D3D9 we don't pack gl_PointSize, since it is
used in a special extra PSIZE register.
Also update some tests to be more robust.
Bug: chromium:804799
Change-Id: I9027266a8b66a28626f038f259bff42ebf09dcd2
Reviewed-on: https://chromium-review.googlesource.com/889898
Commit-Queue: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
diff --git a/src/libANGLE/Caps.cpp b/src/libANGLE/Caps.cpp
index 3779d24..8e3a9c5 100644
--- a/src/libANGLE/Caps.cpp
+++ b/src/libANGLE/Caps.cpp
@@ -253,7 +253,8 @@
attributeZeroRequiresZeroDivisorInEXT(false),
noSeparateStencilRefsAndMasks(false),
shadersRequireIndexedLoopValidation(false),
- noSimultaneousConstantColorAndAlphaBlendFunc(false)
+ noSimultaneousConstantColorAndAlphaBlendFunc(false),
+ noFlexibleVaryingPacking(false)
{
}
diff --git a/src/libANGLE/Caps.h b/src/libANGLE/Caps.h
index 70c8a05..45f0038 100644
--- a/src/libANGLE/Caps.h
+++ b/src/libANGLE/Caps.h
@@ -410,6 +410,9 @@
// 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
diff --git a/src/libANGLE/Program.cpp b/src/libANGLE/Program.cpp
index fffaaab..657bc18 100644
--- a/src/libANGLE/Program.cpp
+++ b/src/libANGLE/Program.cpp
@@ -986,8 +986,16 @@
// 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},
diff --git a/src/libANGLE/VaryingPacking.cpp b/src/libANGLE/VaryingPacking.cpp
index 332b6cb..41c677f 100644
--- a/src/libANGLE/VaryingPacking.cpp
+++ b/src/libANGLE/VaryingPacking.cpp
@@ -69,7 +69,7 @@
// 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 @@
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 @@
// 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(!field.isStruct() && !field.isArray());
- mPackedVaryings.push_back(PackedVarying(field, interpolation, output->name));
+ 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
+ {
+ 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 @@
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 @@
return true;
}
-unsigned int VaryingPacking::getRegisterCount() const
-{
- unsigned int count = 0;
-
- for (const Register ® : mRegisterMap)
- {
- if (reg.data[0] || reg.data[1] || reg.data[2] || reg.data[3])
- {
- ++count;
- }
- }
-
- return count;
-}
-
} // namespace rx
diff --git a/src/libANGLE/VaryingPacking.h b/src/libANGLE/VaryingPacking.h
index e503df2..c02c598 100644
--- a/src/libANGLE/VaryingPacking.h
+++ b/src/libANGLE/VaryingPacking.h
@@ -141,6 +141,9 @@
// 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 @@
{
return static_cast<unsigned int>(mRegisterList.size());
}
- unsigned int getRegisterCount() const;
- size_t getRegisterMapSize() const { return mRegisterMap.size(); }
private:
bool packVarying(const PackedVarying &packedVarying);
diff --git a/src/libANGLE/renderer/d3d/ProgramD3D.cpp b/src/libANGLE/renderer/d3d/ProgramD3D.cpp
index 80ae30b..be1660f 100644
--- a/src/libANGLE/renderer/d3d/ProgramD3D.cpp
+++ b/src/libANGLE/renderer/d3d/ProgramD3D.cpp
@@ -1729,16 +1729,6 @@
}
}
- // 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);
diff --git a/src/libANGLE/renderer/d3d/d3d9/renderer9_utils.cpp b/src/libANGLE/renderer/d3d/d3d9/renderer9_utils.cpp
index fd451a6..c9dc3a5 100644
--- a/src/libANGLE/renderer/d3d/d3d9/renderer9_utils.cpp
+++ b/src/libANGLE/renderer/d3d/d3d9/renderer9_utils.cpp
@@ -292,6 +292,15 @@
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 @@
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 @@
// 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;
}
}
diff --git a/src/libANGLE/renderer/d3d/d3d9/renderer9_utils.h b/src/libANGLE/renderer/d3d/d3d9/renderer9_utils.h
index 5b65b89..bbbf726 100644
--- a/src/libANGLE/renderer/d3d/d3d9/renderer9_utils.h
+++ b/src/libANGLE/renderer/d3d/d3d9/renderer9_utils.h
@@ -48,6 +48,8 @@
namespace d3d9_gl
{
+unsigned int GetReservedVaryingVectors();
+
unsigned int GetReservedVertexUniformVectors();
unsigned int GetReservedFragmentUniformVectors();
diff --git a/src/tests/gl_tests/GLSLTest.cpp b/src/tests/gl_tests/GLSLTest.cpp
index 9066773..c736579 100644
--- a/src/tests/gl_tests/GLSLTest.cpp
+++ b/src/tests/gl_tests/GLSLTest.cpp
@@ -1297,29 +1297,62 @@
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.
+ // Generate shader code that uses gl_FragCoord.
+ VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings - 1, 0, true, false, false, true);
+}
+
+// Verify we can pack registers with two builtin varyings.
+TEST_P(GLSLTest, MaxVaryingVec4_TwoBuiltins)
+{
+ 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);
}
-TEST_P(GLSLTest, MaxMinusTwoVaryingVec4PlusThreeSpecialVariables)
+// Verify we can pack registers with three builtin varyings.
+TEST_P(GLSLTest, MaxVaryingVec4_ThreeBuiltins)
{
- // 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, 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 @@
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 @@
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