<div dir="ltr">On 11 October 2013 11:18, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> 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 <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> Cc: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> --- 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. </blockquote><div> </div><div>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. </div><div>With that changed, this patch is: </div><div>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> </div><div>I've made comments on all the other patches in the series except for patch 4. That patch is: </div><div>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> </div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> */ 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 </blockquote></div> </div></div>