On 18 October 2013 13:40, Paul Berry <stereotype...@gmail.com> wrote:
> 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>
>
This morning I pulled down the latest master (with this patch landed) and
ran the "glsl-1.50" subset of piglit (this is what I've been using as a
smoke test during my geometry shader development). It turns out that some
tests regressed. Since I was encouraging you to do less testing, I wonder
if I'm going to have to eat some of my words :)
The tests that now fail are those that use built-in geometry shader inputs:
- clip-distance-bulk-copy
- clip-distance-in-bulk-read
- clip-distance-in-explicitly-sized
- clip-distance-in-param
- clip-distance-in-values
- core-inputs
- gs-redeclares-both-pervertex-blocks
- gs-redeclares-pervertex-in-only
- gs-redeclares-pervertex-out-only
- redeclare-pervertex-subset-vs-to-gs
- unsized-in-named-interface-block-gs
- unsized-in-named-interface-block-multiple
- unsized-in-unnamed-interface-block-gs
- unsized-in-unnamed-interface-block-multiple
It seems that we both missed a mechanism by which built-in variables could
be created which have explicit_location set to false: when a built-in
variable is created by lower_named_interface_blocks. The test I encouraged
you to remove wouldn't have caught this problem, and I'm having trouble
thinking of a unit test or assertion that would have caught it.
Fortunately piglit caught it :)
I'll post a patch in a few minutes which fixes the regression by causing
lower_named_interface_blocks to set explicit_location = true whenever it's
creating a variable with a location >= 0.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
