On 10/22/2015 09:41 AM, Timothy Arceri wrote:
On Thu, 2015-10-22 at 08:55 +0300, Tapani Pälli wrote:
On 10/22/2015 08:29 AM, Timothy Arceri wrote:
Location has never been able to be a negative value because it has
always been validated in the parser.
Also the linker doesn't check for negatives like the comment
claims.
Neither does the parser, if one utilizes negative explicit location,
parser says:
error: syntax error, unexpected '-', expecting INTCONSTANT or
UINTCONSTANT
I'm not sure if this is quite OK, it should rather accept the
negative
value and the fail here in this check you are about to remove?
Yes I did noticed this. However even if we changed it to accept a
negative value the is another check in the parser that would catch this
before the checks I'm removing here.
if ($3 >= 0) {
$$.location = $3;
} else {
_mesa_glsl_error(& @3, state, "invalid location %d specified", $3);
YYERROR;
}
OK, then removing the check for uniform loc should be fine. For the
attributes change, I'm not sure yet, I believe the reason for the
'silliness' (mentioned in the commit) is that all the built-in
attributes have negative locations, we need to be careful to continue
dealing gracefully with that.
I'm also working on ARB_enhanced_layouts which will allow negative
values to get passed the parser and will be moving the check out of the
parser and into the ast.
This patch is a clean-up before doing that, as the attributes code
doesn't do the validation at all and the one for uniforms should be
shared with the attibutes code.
---
No piglit regressions and an extra negative test sent for
ARB_explicit_uniform_location [1]
[1] http://patchwork.freedesktop.org/patch/62573/
src/glsl/ast_to_hir.cpp | 70 ++++++++++++++++--------------------
-------------
1 file changed, 22 insertions(+), 48 deletions(-)
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
index 8549d55..0306530 100644
--- a/src/glsl/ast_to_hir.cpp
+++ b/src/glsl/ast_to_hir.cpp
@@ -2422,21 +2422,6 @@ validate_explicit_location(const struct
ast_type_qualifier *qual,
const struct gl_context *const ctx = state->ctx;
unsigned max_loc = qual->location + var->type
->uniform_locations() - 1;
- /* ARB_explicit_uniform_location specification states:
- *
- * "The explicitly defined locations and the generated
locations
- * must be in the range of 0 to MAX_UNIFORM_LOCATIONS
minus one."
- *
- * "Valid locations for default-block uniform variable
locations
- * are in the range of 0 to the implementation-defined
maximum
- * number of uniform locations."
- */
- if (qual->location < 0) {
- _mesa_glsl_error(loc, state,
- "explicit location < 0 for uniform %s",
var->name);
- return;
- }
-
if (max_loc >= ctx
->Const.MaxUserAssignableUniformLocations) {
_mesa_glsl_error(loc, state, "location(s) consumed by
uniform %s "
">= MAX_UNIFORM_LOCATIONS (%u)", var
->name,
@@ -2527,41 +2512,30 @@ validate_explicit_location(const struct
ast_type_qualifier *qual,
} else {
var->data.explicit_location = true;
- /* This bit of silliness is needed because invalid explicit
locations
- * are supposed to be flagged during linking. Small
negative values
- * biased by VERT_ATTRIB_GENERIC0 or FRAG_RESULT_DATA0 could
alias
- * built-in values (e.g., -16+VERT_ATTRIB_GENERIC0 =
VERT_ATTRIB_POS).
- * The linker needs to be able to differentiate these cases.
This
- * ensures that negative values stay negative.
- */
- if (qual->location >= 0) {
- switch (state->stage) {
- case MESA_SHADER_VERTEX:
- var->data.location = (var->data.mode ==
ir_var_shader_in)
- ? (qual->location + VERT_ATTRIB_GENERIC0)
- : (qual->location + VARYING_SLOT_VAR0);
- break;
+ switch (state->stage) {
+ case MESA_SHADER_VERTEX:
+ var->data.location = (var->data.mode == ir_var_shader_in)
+ ? (qual->location + VERT_ATTRIB_GENERIC0)
+ : (qual->location + VARYING_SLOT_VAR0);
+ break;
- case MESA_SHADER_TESS_CTRL:
- case MESA_SHADER_TESS_EVAL:
- case MESA_SHADER_GEOMETRY:
- if (var->data.patch)
- var->data.location = qual->location +
VARYING_SLOT_PATCH0;
- else
- var->data.location = qual->location +
VARYING_SLOT_VAR0;
- break;
+ case MESA_SHADER_TESS_CTRL:
+ case MESA_SHADER_TESS_EVAL:
+ case MESA_SHADER_GEOMETRY:
+ if (var->data.patch)
+ var->data.location = qual->location +
VARYING_SLOT_PATCH0;
+ else
+ var->data.location = qual->location +
VARYING_SLOT_VAR0;
+ break;
- case MESA_SHADER_FRAGMENT:
- var->data.location = (var->data.mode ==
ir_var_shader_out)
- ? (qual->location + FRAG_RESULT_DATA0)
- : (qual->location + VARYING_SLOT_VAR0);
- break;
- case MESA_SHADER_COMPUTE:
- assert(!"Unexpected shader type");
- break;
- }
- } else {
- var->data.location = qual->location;
+ case MESA_SHADER_FRAGMENT:
+ var->data.location = (var->data.mode ==
ir_var_shader_out)
+ ? (qual->location + FRAG_RESULT_DATA0)
+ : (qual->location + VARYING_SLOT_VAR0);
+ break;
+ case MESA_SHADER_COMPUTE:
+ assert(!"Unexpected shader type");
+ break;
}
if (qual->flags.q.explicit_index) {
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
