On 11 October 2013 11:18, Ian Romanick <i...@freedesktop.org> wrote:
> From: Ian Romanick <ian.d.roman...@intel.com>
>
> The unit tests added in the previous commits prove some things about the
> state of some internal data structures. The most important of these is
> that all built-in input and output variables have explicit_location
> set. This means that link_invalidate_variable_locations doesn't need to
> know the range of non-generic shader inputs or outputs. It can simply
> reset location state depending on whether explicit_location is set.
>
> There are two additional assumptions that were already implicit in the
> code that comments now document.
>
> - ir_variable::is_unmatched_generic_inout is only used by the linker
> when connecting outputs from one shader stage to inputs of another
> shader stage.
>
> - Any varying that has explicit_location set must be a built-in. This
> will be true until GL_ARB_separate_shader_objects is supported.
>
> As a result, the input_base and output_base parameters to
> link_invalidate_variable_locations are no longer necessary, and the code
> for resetting locations and setting is_unmatched_generic_inout can be
> simplified.
>
> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
> Cc: Paul Berry <stereotype...@gmail.com>
> ---
> src/glsl/linker.cpp | 48 ++++++++++-----------
> src/glsl/linker.h | 3 +-
> src/glsl/tests/invalidate_locations_test.cpp | 62
> ++++++++++++++--------------
> 3 files changed, 55 insertions(+), 58 deletions(-)
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 02eb4e1..acc3039 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -366,8 +366,7 @@ parse_program_resource_name(const GLchar *name,
>
>
> void
> -link_invalidate_variable_locations(exec_list *ir, int input_base,
> - int output_base)
> +link_invalidate_variable_locations(exec_list *ir)
> {
> foreach_list(node, ir) {
> ir_variable *const var = ((ir_instruction *) node)->as_variable();
> @@ -375,26 +374,30 @@ link_invalidate_variable_locations(exec_list *ir,
> int input_base,
> if (var == NULL)
> continue;
>
> - int base;
> - switch (var->mode) {
> - case ir_var_shader_in:
> - base = input_base;
> - break;
> - case ir_var_shader_out:
> - base = output_base;
> - break;
> - default:
> - continue;
> - }
> -
> - /* Only assign locations for generic attributes / varyings / etc.
> + /* Only assign locations for variables that lack an explicit
> location.
> + * Explicit locations are set for all built-in variables, generic
> vertex
> + * shader inputs (via layout(location=...)), and generic fragment
> shader
> + * outputs (also via layout(location=...)).
> */
> - if ((var->location >= base) && !var->explicit_location)
> + if (!var->explicit_location) {
> var->location = -1;
> + var->location_frac = 0;
> + }
>
> - if ((var->location == -1) && !var->explicit_location) {
> + /* ir_variable::is_unmatched_generic_inout is used by the linker
> while
> + * connecting outputs from one stage to inputs of the next stage.
> + *
> + * There are two implicit assumptions here. First, we assume that
> any
> + * built-in variable (i.e., non-generic in or out) will have
> + * explicit_location set. Second, we assume that any generic in or
> out
> + * will not have explicit_location set.
> + *
> + * This second assumption will only be valid until
> + * GL_ARB_separate_shader_objects is supported. When that
> extension is
> + * implemented, this function will need some modifications.
> + */
> + if (!var->explicit_location) {
> var->is_unmatched_generic_inout = 1;
> - var->location_frac = 0;
> } else {
> var->is_unmatched_generic_inout = 0;
> }
> @@ -2221,18 +2224,15 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
> /* Mark all generic shader inputs and outputs as unpaired. */
> if (prog->_LinkedShaders[MESA_SHADER_VERTEX] != NULL) {
> link_invalidate_variable_locations(
> - prog->_LinkedShaders[MESA_SHADER_VERTEX]->ir,
> - VERT_ATTRIB_GENERIC0, VARYING_SLOT_VAR0);
> + prog->_LinkedShaders[MESA_SHADER_VERTEX]->ir);
> }
> if (prog->_LinkedShaders[MESA_SHADER_GEOMETRY] != NULL) {
> link_invalidate_variable_locations(
> - prog->_LinkedShaders[MESA_SHADER_GEOMETRY]->ir,
> - VARYING_SLOT_VAR0, VARYING_SLOT_VAR0);
> + prog->_LinkedShaders[MESA_SHADER_GEOMETRY]->ir);
> }
> if (prog->_LinkedShaders[MESA_SHADER_FRAGMENT] != NULL) {
> link_invalidate_variable_locations(
> - prog->_LinkedShaders[MESA_SHADER_FRAGMENT]->ir,
> - VARYING_SLOT_VAR0, FRAG_RESULT_DATA0);
> + prog->_LinkedShaders[MESA_SHADER_FRAGMENT]->ir);
> }
>
> /* FINISHME: The value of the max_attribute_index parameter is
> diff --git a/src/glsl/linker.h b/src/glsl/linker.h
> index 9915c38..887cd33 100644
> --- a/src/glsl/linker.h
> +++ b/src/glsl/linker.h
> @@ -31,8 +31,7 @@ link_function_calls(gl_shader_program *prog, gl_shader
> *main,
> gl_shader **shader_list, unsigned num_shaders);
>
> extern void
> -link_invalidate_variable_locations(exec_list *ir, int input_base,
> - int output_base);
> +link_invalidate_variable_locations(exec_list *ir);
>
> extern void
> link_assign_uniform_locations(struct gl_shader_program *prog);
> diff --git a/src/glsl/tests/invalidate_locations_test.cpp
> b/src/glsl/tests/invalidate_locations_test.cpp
> index 958acec..b26d825 100644
> --- a/src/glsl/tests/invalidate_locations_test.cpp
> +++ b/src/glsl/tests/invalidate_locations_test.cpp
> @@ -72,9 +72,7 @@ TEST_F(invalidate_locations, simple_vertex_in_generic)
>
> ir.push_tail(var);
>
> - link_invalidate_variable_locations(&ir,
> - VERT_ATTRIB_GENERIC0,
> - VARYING_SLOT_VAR0);
> + link_invalidate_variable_locations(&ir);
>
> EXPECT_EQ(-1, var->location);
> EXPECT_EQ(0u, var->location_frac);
> @@ -97,9 +95,7 @@ TEST_F(invalidate_locations,
> explicit_location_vertex_in_generic)
>
> ir.push_tail(var);
>
> - link_invalidate_variable_locations(&ir,
> - VERT_ATTRIB_GENERIC0,
> - VARYING_SLOT_VAR0);
> + link_invalidate_variable_locations(&ir);
>
> EXPECT_EQ(VERT_ATTRIB_GENERIC0, var->location);
> EXPECT_EQ(0u, var->location_frac);
> @@ -123,9 +119,7 @@ TEST_F(invalidate_locations,
> explicit_location_frac_vertex_in_generic)
>
> ir.push_tail(var);
>
> - link_invalidate_variable_locations(&ir,
> - VERT_ATTRIB_GENERIC0,
> - VARYING_SLOT_VAR0);
> + link_invalidate_variable_locations(&ir);
>
> EXPECT_EQ(VERT_ATTRIB_GENERIC0, var->location);
> EXPECT_EQ(2u, var->location_frac);
> @@ -148,9 +142,7 @@ TEST_F(invalidate_locations, vertex_in_builtin)
>
> ir.push_tail(var);
>
> - link_invalidate_variable_locations(&ir,
> - VERT_ATTRIB_GENERIC0,
> - VARYING_SLOT_VAR0);
> + link_invalidate_variable_locations(&ir);
>
> EXPECT_EQ(VERT_ATTRIB_POS, var->location);
> EXPECT_EQ(0u, var->location_frac);
> @@ -162,8 +154,15 @@ TEST_F(invalidate_locations,
> vertex_in_builtin_without_explicit)
> {
> /* This test is almost identical to vertex_in_builtin. However,
> * ir_variable::explicit_location is not.
> - * link_invalidate_variable_locations has the behavior that non-generic
> - * inputs (or outputs) are not modified.
> + *
> + * At one point in time, link_invalidate_variable_locations would not
> + * modify any non-generic inputs (or outputs). However, the
> + * vertex_builtin::inputs_have_explicit_location et al. test cases
> prove
> + * that all built-in variables have explicit_location set.
> + * link_invalidate_variable_locations will reset the locations of any
> + * variable that does not have explicit_location set.
> + *
> + * In addition, is_unmatch_generic_inout depend only on
> explicit_location.
>
Basically what you're saying here is "this corner case that never happens
now has different behaviour, so we have to change the test." As I
mentioned in my comments on patch 5, I would prefer to just see the
*_builtin_without_explicit tests go away--there's no point in testing a
case that cannot occur, especially when there are other tests to verify
that the case cannot occur. That would make this patch a lot simpler,
since we wouldn't have to contort ourselves to explain why we need to
change a test when the functionality is preserved.
With that changed, this patch is:
Reviewed-by: Paul Berry <stereotype...@gmail.com>
I've made comments on all the other patches in the series except for patch
4. That patch is:
Reviewed-by: Paul Berry <stereotype...@gmail.com>
> */
> ir_variable *const var =
> new(mem_ctx) ir_variable(glsl_type::vec(4),
> @@ -177,14 +176,12 @@ TEST_F(invalidate_locations,
> vertex_in_builtin_without_explicit)
>
> ir.push_tail(var);
>
> - link_invalidate_variable_locations(&ir,
> - VERT_ATTRIB_GENERIC0,
> - VARYING_SLOT_VAR0);
> + link_invalidate_variable_locations(&ir);
>
> - EXPECT_EQ(VERT_ATTRIB_POS, var->location);
> + EXPECT_EQ(-1, var->location);
> EXPECT_EQ(0u, var->location_frac);
> EXPECT_FALSE(var->explicit_location);
> - EXPECT_FALSE(var->is_unmatched_generic_inout);
> + EXPECT_TRUE(var->is_unmatched_generic_inout);
> }
>
> TEST_F(invalidate_locations, simple_vertex_out_generic)
> @@ -201,9 +198,7 @@ TEST_F(invalidate_locations, simple_vertex_out_generic)
>
> ir.push_tail(var);
>
> - link_invalidate_variable_locations(&ir,
> - VERT_ATTRIB_GENERIC0,
> - VARYING_SLOT_VAR0);
> + link_invalidate_variable_locations(&ir);
>
> EXPECT_EQ(-1, var->location);
> EXPECT_EQ(0u, var->location_frac);
> @@ -226,9 +221,7 @@ TEST_F(invalidate_locations, vertex_out_builtin)
>
> ir.push_tail(var);
>
> - link_invalidate_variable_locations(&ir,
> - VERT_ATTRIB_GENERIC0,
> - VARYING_SLOT_VAR0);
> + link_invalidate_variable_locations(&ir);
>
> EXPECT_EQ(VARYING_SLOT_COL0, var->location);
> EXPECT_EQ(0u, var->location_frac);
> @@ -240,8 +233,15 @@ TEST_F(invalidate_locations,
> vertex_out_builtin_without_explicit)
> {
> /* This test is almost identical to vertex_out_builtin. However,
> * ir_variable::explicit_location is not.
> - * link_invalidate_variable_locations has the behavior that non-generic
> - * inputs (or outputs) are not modified.
> + *
> + * At one point in time, link_invalidate_variable_locations would not
> + * modify any non-generic inputs (or outputs). However, the
> + * vertex_builtin::inputs_have_explicit_location et al. test cases
> prove
> + * that all built-in variables have explicit_location set.
> + * link_invalidate_variable_locations will reset the locations of any
> + * variable that does not have explicit_location set.
> + *
> + * In addition, is_unmatch_generic_inout depend only on
> explicit_location.
> */
> ir_variable *const var =
> new(mem_ctx) ir_variable(glsl_type::vec(4),
> @@ -255,12 +255,10 @@ TEST_F(invalidate_locations,
> vertex_out_builtin_without_explicit)
>
> ir.push_tail(var);
>
> - link_invalidate_variable_locations(&ir,
> - VERT_ATTRIB_GENERIC0,
> - VARYING_SLOT_VAR0);
> + link_invalidate_variable_locations(&ir);
>
> - EXPECT_EQ(VARYING_SLOT_COL0, var->location);
> + EXPECT_EQ(-1, var->location);
> EXPECT_EQ(0u, var->location_frac);
> EXPECT_FALSE(var->explicit_location);
> - EXPECT_FALSE(var->is_unmatched_generic_inout);
> + EXPECT_TRUE(var->is_unmatched_generic_inout);
> }
> --
> 1.8.1.4
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
