Compiler: Allow deferred array sizing in geometry shaders
Based on work by Brandon Schade <b.schade@samsung.com>.
When a shader sets an array input size in a GS after declaring input
variables, compilation would fail.
Example shader -
in vec3 normal[];
in vec3 view_dir[];
in float patch_dist[];
in float e_patch_wire_scale[];
...
layout(triangles) in;
layout(triangle_strip, max_vertices=3) out;
void main() {
...
Update translator to handle such cases.
Also add a new check that there are no remaining unsized
arrays when compilation has completed.
Test: GeometryShaderTest.DeferredSetOfArrayInputSize
Bug: angleproject:3571
Bug: angleproject:7125
Change-Id: I4853832c27f9551284bcca92b98cbf5f3a63aaf5
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3564259
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Commit-Queue: Cody Northrop <cnorthrop@google.com>
diff --git a/src/compiler/translator/Compiler.cpp b/src/compiler/translator/Compiler.cpp
index 62d231c..01cae10 100644
--- a/src/compiler/translator/Compiler.cpp
+++ b/src/compiler/translator/Compiler.cpp
@@ -461,7 +461,7 @@
return nullptr;
}
- if (parseContext.getTreeRoot() == nullptr)
+ if (!postParseChecks(parseContext))
{
return nullptr;
}
@@ -1119,6 +1119,29 @@
return true;
}
+bool TCompiler::postParseChecks(const TParseContext &parseContext)
+{
+ std::stringstream errorMessage;
+
+ if (parseContext.getTreeRoot() == nullptr)
+ {
+ errorMessage << "Shader parsing failed (mTreeRoot == nullptr)";
+ }
+
+ for (TType *type : parseContext.getDeferredArrayTypesToSize())
+ {
+ errorMessage << "Unsized global array type: " << type->getBasicString();
+ }
+
+ if (!errorMessage.str().empty())
+ {
+ mDiagnostics.globalError(errorMessage.str().c_str());
+ return false;
+ }
+
+ return true;
+}
+
bool TCompiler::compile(const char *const shaderStrings[],
size_t numStrings,
ShCompileOptions compileOptionsIn)
diff --git a/src/compiler/translator/Compiler.h b/src/compiler/translator/Compiler.h
index f3ca016..6f90b19 100644
--- a/src/compiler/translator/Compiler.h
+++ b/src/compiler/translator/Compiler.h
@@ -291,6 +291,8 @@
const TParseContext &parseContext,
ShCompileOptions compileOptions);
+ bool postParseChecks(const TParseContext &parseContext);
+
sh::GLenum mShaderType;
ShShaderSpec mShaderSpec;
ShShaderOutput mOutputType;
diff --git a/src/compiler/translator/ParseContext.cpp b/src/compiler/translator/ParseContext.cpp
index 20788cb..80226fb 100644
--- a/src/compiler/translator/ParseContext.cpp
+++ b/src/compiler/translator/ParseContext.cpp
@@ -1238,9 +1238,11 @@
}
}
- // Implicitly declared arrays are disallowed for shaders other than tessellation shaders.
- if (mShaderType != GL_TESS_CONTROL_SHADER && mShaderType != GL_TESS_EVALUATION_SHADER &&
- type->isArray())
+ // Implicitly declared arrays are only allowed with tessellation or geometry shader inputs
+ if (type->isArray() &&
+ ((mShaderType != GL_TESS_CONTROL_SHADER && mShaderType != GL_TESS_EVALUATION_SHADER &&
+ mShaderType != GL_GEOMETRY_SHADER) ||
+ (mShaderType == GL_GEOMETRY_SHADER && type->getQualifier() == EvqGeometryOut)))
{
const TSpan<const unsigned int> &arraySizes = type->getArraySizes();
for (unsigned int size : arraySizes)
@@ -1248,8 +1250,8 @@
if (size == 0)
{
error(line,
- "implicitly sized arrays disallowed for shaders that are not tessellation "
- "shaders",
+ "implicitly sized arrays only allowed for tessellation shaders "
+ "or geometry shader inputs",
identifier);
}
}
@@ -2818,10 +2820,11 @@
// [GLSL ES 3.2 SPEC Chapter 4.4.1.2]
// An input can be declared without an array size if there is a previous layout
// which specifies the size.
- error(location,
- "Missing a valid input primitive declaration before declaring an unsized "
- "array input",
- token);
+ warning(location,
+ "Missing a valid input primitive declaration before declaring an unsized "
+ "array input",
+ "Deferred");
+ mDeferredArrayTypesToSize.push_back(type);
}
}
else if (type->isArray())
@@ -2881,7 +2884,7 @@
// declared, this is deferred until such time as it does.
if (mTessControlShaderOutputVertices == 0)
{
- mTessControlDeferredArrayTypesToSize.push_back(type);
+ mDeferredArrayTypesToSize.push_back(type);
}
else
{
@@ -3449,6 +3452,14 @@
"layout");
return false;
}
+
+ // Size any implicitly sized arrays that have already been declared.
+ for (TType *type : mDeferredArrayTypesToSize)
+ {
+ type->sizeOutermostUnsizedArray(
+ symbolTable.getGlInVariableWithArraySize()->getType().getOutermostArraySize());
+ }
+ mDeferredArrayTypesToSize.clear();
}
// Set mGeometryInvocations if exists
@@ -3539,10 +3550,11 @@
mTessControlShaderOutputVertices = layoutQualifier.vertices;
// Size any implicitly sized arrays that have already been declared.
- for (TType *type : mTessControlDeferredArrayTypesToSize)
+ for (TType *type : mDeferredArrayTypesToSize)
{
type->sizeOutermostUnsizedArray(mTessControlShaderOutputVertices);
}
+ mDeferredArrayTypesToSize.clear();
}
else
{
diff --git a/src/compiler/translator/ParseContext.h b/src/compiler/translator/ParseContext.h
index 8057697..fa3e56a 100644
--- a/src/compiler/translator/ParseContext.h
+++ b/src/compiler/translator/ParseContext.h
@@ -499,6 +499,11 @@
return mTessEvaluationShaderInputPointType;
}
+ const TVector<TType *> &getDeferredArrayTypesToSize() const
+ {
+ return mDeferredArrayTypesToSize;
+ }
+
void markShaderHasPrecise() { mHasAnyPreciseType = true; }
bool hasAnyPreciseType() const { return mHasAnyPreciseType; }
AdvancedBlendEquations getAdvancedBlendEquations() const { return mAdvancedBlendEquations; }
@@ -738,7 +743,7 @@
TLayoutTessEvaluationType mTessEvaluationShaderInputPointType;
// List of array declarations without an explicit size that have come before layout(vertices=N).
// Once the vertex count is specified, these arrays are sized.
- TVector<TType *> mTessControlDeferredArrayTypesToSize;
+ TVector<TType *> mDeferredArrayTypesToSize;
// Whether the |precise| keyword has been seen in the shader.
bool mHasAnyPreciseType;
diff --git a/src/tests/compiler_tests/GeometryShader_test.cpp b/src/tests/compiler_tests/GeometryShader_test.cpp
index 14c0624..f69120e 100644
--- a/src/tests/compiler_tests/GeometryShader_test.cpp
+++ b/src/tests/compiler_tests/GeometryShader_test.cpp
@@ -1220,8 +1220,8 @@
}
}
-// Verify that it is a compile error to declare an unsized Geometry Shader input before a valid
-// input primitive declaration.
+// Verify that compilation errors do not occur even if declaring an unsized Geometry Shader input
+// before a valid input primitive declaration.
TEST_F(GeometryShaderTest, DeclareUnsizedInputBeforeInputPrimitive)
{
const std::string &shaderString1 =
@@ -1249,9 +1249,9 @@
int length = texcoord2.length();
})";
- if (compile(shaderString1) || compile(shaderString2))
+ if (!compile(shaderString1) || !compile(shaderString2))
{
- FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+ FAIL() << "Shader compilation failed, expecting success:\n" << mInfoLog;
}
}
diff --git a/src/tests/gl_tests/GeometryShaderTest.cpp b/src/tests/gl_tests/GeometryShaderTest.cpp
index 41ebfbe..6ac89d2 100644
--- a/src/tests/gl_tests/GeometryShaderTest.cpp
+++ b/src/tests/gl_tests/GeometryShaderTest.cpp
@@ -144,6 +144,42 @@
EXPECT_GL_NO_ERROR();
}
+// Verify that Geometry Shader can be compiled when geometry shader array input size
+// is set after shader input variables.
+// http://anglebug.com/7125 GFXBench Car Chase uses this pattern
+TEST_P(GeometryShaderTest, DeferredSetOfArrayInputSize)
+{
+ ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_geometry_shader"));
+ // "layout (invocations = 3, triangles) in;" should be declared before "in vec4 texcoord [];",
+ // but here it is declared later.
+ constexpr char kGS[] = R"(#version 310 es
+#extension GL_EXT_geometry_shader : require
+in vec4 texcoord[];
+out vec4 o_texcoord;
+layout (invocations = 3, triangles_adjacency) in;
+layout (triangle_strip, max_vertices = 3) out;
+void main()
+{
+ int n;
+ for (n = 0; n < gl_in.length(); n++)
+ {
+ gl_Position = gl_in[n].gl_Position;
+ gl_Layer = gl_InvocationID;
+ o_texcoord = texcoord[n];
+ EmitVertex();
+ }
+ EndPrimitive();
+})";
+ GLuint geometryShader = CompileShader(GL_GEOMETRY_SHADER_EXT, kGS);
+ EXPECT_NE(0u, geometryShader);
+ GLuint programID = glCreateProgram();
+ glAttachShader(programID, geometryShader);
+ glDetachShader(programID, geometryShader);
+ glDeleteShader(geometryShader);
+ glDeleteProgram(programID);
+ EXPECT_GL_NO_ERROR();
+}
+
// Verify that all the implementation dependent geometry shader related resource limits meet the
// requirement of GL_EXT_geometry_shader SPEC.
TEST_P(GeometryShaderTest, GeometryShaderImplementationDependentLimits)
