On 10/22/2015 11:07 AM, Timothy Arceri wrote:
On Thu, 2015-10-22 at 09:56 +0300, Tapani Pälli wrote:
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.
This function is only valdating the qualifier, builtins don't seem to
set the qualifier flags in the ast, as far as I can tell they set the
fields in the ir directly.
For location the only place that sets the explicit location flag is the
parser and this validation function is only called when that is set, so
I don't believe this should be a problem.
if (qual->flags.q.explicit_location) {
validate_explicit_location(qual, var, state, loc);
}
Yep, this is the case. Builtin variables have explicit locations but
those are set directly to the ir_variable when creating them. I can't
find any way of them being negative here (especially as parser just hits
syntax error with both attributes and uniforms for locations < 0):
Reviewed-by: Tapani Pälli <[email protected]>
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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
