On 23/05/18 03:16, Ian Romanick wrote:
On 05/11/2018 09:49 PM, Timothy Arceri wrote:
Mesa seems to be the only implementation that doesn't allow builtins
to be explicitly declared. The GLSL 1.30 spec seems to imply that
buitins may be explicitly declared.
There was a clarification added to the spec in response to a Khronos bug
that I submitted. I don't recall the version where it was added, but
the resulting text is in section 3.7 (Identifiers):
Identifiers starting with “gl_” are reserved for use by OpenGL, and
may not be declared in a shader; this results in a compile-time
error. However, as noted in the specification, there are some cases
where previously declared variables can be redeclared, and
predeclared "gl_" names are allowed to be redeclared in a shader
only for these specific purposes. More generally, it is a
compile-time error to redeclare a variable, including those starting
“gl_”.
The specific cases where redeclaration is allowed are cases where
invariant is added, precision qualifiers are added, an array size is
added, etc.
This this allows the game "Full Bore" the be playable (when using
MESA_GL_VERSION_OVERRIDE=3.3COMPAT). It will also allow us to
remove the allow_glsl_builtin_variable_redeclaration dri override.
What is the nature of the redeclaration in this game?
It explicitly declares the builtin as it is specified in the spec:
#version 130
out vec4 gl_FragColor;
I'm assuming it was done to get rid of warnings that the builtin has
been depreciated. This seems to be inline with the spec quote:
From the GLSL 1.30 spec Section 7.2 (Fragment Shader Special
Variables):
"Both gl_FragColor and gl_FragData are deprecated; the preferred
usage is to explicitly declare these outputs in the fragment
shader using the out storage qualifier."
Does the shader
in question compile with glslang?
No it doesn't. However its a little sad that the spec was updated to
specifically disallow explicit declaration of builtins but we have 0
piglit tests and 0 CTS tests for it. Especially since every other
implementation allows it.
From the GLSL 1.30 spec Section 7.2 (Fragment Shader Special
Variables):
"Both gl_FragColor and gl_FragData are deprecated; the preferred
usage is to explicitly declare these outputs in the fragment
shader using the out storage qualifier."
To avoid some GLSL ES tests failing we add a check to make sure
precision matches on the redeclared builtin.
---
src/compiler/glsl/ast_to_hir.cpp | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index a7a9ac80769..54d0816a986 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -4390,14 +4390,8 @@ get_variable_being_redeclared(ir_variable **var_ptr,
YYLTYPE loc,
earlier->data.precision = var->data.precision;
earlier->data.memory_coherent = var->data.memory_coherent;
- } else if (earlier->data.how_declared == ir_var_declared_implicitly &&
- state->allow_builtin_variable_redeclaration) {
- /* Allow verbatim redeclarations of built-in variables. Not explicitly
- * valid, but some applications do it.
- */
- if (earlier->data.mode != var->data.mode &&
- !(earlier->data.mode == ir_var_system_value &&
- var->data.mode == ir_var_shader_in)) {
+ } else if (allow_all_redeclarations) {
+ if (earlier->data.mode != var->data.mode) {
_mesa_glsl_error(&loc, state,
"redeclaration of `%s' with incorrect qualifiers",
var->name);
@@ -4406,8 +4400,22 @@ get_variable_being_redeclared(ir_variable **var_ptr,
YYLTYPE loc,
"redeclaration of `%s' has incorrect type",
var->name);
}
- } else if (allow_all_redeclarations) {
- if (earlier->data.mode != var->data.mode) {
+ } else if (earlier->data.how_declared == ir_var_declared_implicitly) {
+ /* Allow verbatim redeclarations of built-in variables. The GLSL 1.30
+ * spec seems to imply that buitins may be explicitly declared.
+ *
+ * From the GLSL 1.30 spec Section 7.2 (Fragment Shader Special
+ * Variables):
+ *
+ * "Both gl_FragColor and gl_FragData are deprecated; the preferred
+ * usage is to explicitly declare these outputs in the fragment
+ * shader using the out storage qualifier."
+ */
+ enum ir_variable_mode builtin_mode =
+ glsl_external_mode((ir_variable_mode) earlier->data.mode,
+ state->stage, earlier->data.location);
+
+ if (builtin_mode != var->data.mode) {
_mesa_glsl_error(&loc, state,
"redeclaration of `%s' with incorrect qualifiers",
var->name);
@@ -4415,6 +4423,10 @@ get_variable_being_redeclared(ir_variable **var_ptr,
YYLTYPE loc,
_mesa_glsl_error(&loc, state,
"redeclaration of `%s' has incorrect type",
var->name);
+ } else if (earlier->data.precision != var->data.precision) {
+ _mesa_glsl_error(&loc, state,
+ "redeclaration of `%s' has incorrect precision",
+ var->name);
}
} else {
_mesa_glsl_error(&loc, state, "`%s' redeclared", var->name);
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
