D3D11: Fix varying packing with structs.
Previously we would try to pass an entire struct through HLSL's
shader interface. Instead, split this off as if each field was
its own variable, which seems to be spec compliant (see ESSL 3.10).
In the future we may want to fix register packing to use specific
components of float4/int4/uint4 HLSL registers. This could also fix
the remaining bugs in the SM3 packing.
TEST=dEQP-GLES3.functional.shaders.linkage.varying.*
BUG=angleproject:910
BUG=angleproject:1202
Change-Id: I1fd8b4505abc39bd2385ed5c088c316d55d0bc2c
Reviewed-on: https://chromium-review.googlesource.com/311242
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Tested-by: Jamie Madill <jmadill@chromium.org>
diff --git a/src/libANGLE/Shader.cpp b/src/libANGLE/Shader.cpp
index 281fcd9..b92e06b 100644
--- a/src/libANGLE/Shader.cpp
+++ b/src/libANGLE/Shader.cpp
@@ -51,7 +51,7 @@
} // 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 @@
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));
}
diff --git a/src/libANGLE/Shader.h b/src/libANGLE/Shader.h
index 4cbe464..7d846a2 100644
--- a/src/libANGLE/Shader.h
+++ b/src/libANGLE/Shader.h
@@ -137,7 +137,7 @@
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_
diff --git a/src/libANGLE/renderer/d3d/DynamicHLSL.cpp b/src/libANGLE/renderer/d3d/DynamicHLSL.cpp
index c8e8e12..1d07b95 100644
--- a/src/libANGLE/renderer/d3d/DynamicHLSL.cpp
+++ b/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 @@
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 @@
for (const PackedVaryingRegister ®isterInfo : 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 @@
// 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 @@
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 @@
for (const PackedVaryingRegister ®isterInfo : 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 << " = _" + varying.name;
+ vertexStream << " output.v" << registerInfo.semanticIndex << " = ";
+
+ 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 @@
for (const PackedVaryingRegister ®isterInfo : 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 @@
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";
}
diff --git a/src/libANGLE/renderer/d3d/ProgramD3D.cpp b/src/libANGLE/renderer/d3d/ProgramD3D.cpp
index 58ac5f8..30020b7 100644
--- a/src/libANGLE/renderer/d3d/ProgramD3D.cpp
+++ b/src/libANGLE/renderer/d3d/ProgramD3D.cpp
@@ -104,7 +104,7 @@
// 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 @@
{
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 @@
{
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 @@
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 @@
{
for (const PackedVaryingRegister ®isterInfo : 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)
diff --git a/src/libANGLE/renderer/d3d/VaryingPacking.cpp b/src/libANGLE/renderer/d3d/VaryingPacking.cpp
index 771bef1..f2654d3 100644
--- a/src/libANGLE/renderer/d3d/VaryingPacking.cpp
+++ b/src/libANGLE/renderer/d3d/VaryingPacking.cpp
@@ -57,23 +57,16 @@
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 @@
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 @@
// 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 @@
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)
diff --git a/src/libANGLE/renderer/d3d/VaryingPacking.h b/src/libANGLE/renderer/d3d/VaryingPacking.h
index 450a50f..ca4640b 100644
--- a/src/libANGLE/renderer/d3d/VaryingPacking.h
+++ b/src/libANGLE/renderer/d3d/VaryingPacking.h
@@ -19,12 +19,32 @@
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)
+ {
+ }
- const sh::Varying *varying;
+ bool isStructField() const { return !parentStructName.empty(); }
+
+ 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 @@
return registerRow * 4 + registerColumn;
}
+ bool isStructField() const { return !structFieldName.empty(); }
+
// Index to the array of varyings.
const PackedVarying *packedVarying;
@@ -69,6 +91,9 @@
// Assigned after packing
unsigned int semanticIndex;
+
+ // Struct member this varying corresponds to.
+ std::string structFieldName;
};
class VaryingPacking final : angle::NonCopyable
diff --git a/src/tests/deqp_support/deqp_gles3_test_expectations.txt b/src/tests/deqp_support/deqp_gles3_test_expectations.txt
index 1ec062c..8216b37 100644
--- a/src/tests/deqp_support/deqp_gles3_test_expectations.txt
+++ b/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