[Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
The existing code was checking the whole interface variable rather
than its members, which is not what we want: we want to check
aliasing for each member in the interface variable.
Surprisingly, there are piglit tests that verify this and were
passing due to a bug in the existing code: when we were computing
the last component used by an interface variable we would use
the 'vector' path and multiply by vector_elements, which is 0 for
interface variables. This made the loop that checks for aliasing
be a no-op and not add the interface variable to the list of outputs
so then we would fail to link when we did not see a matching output
for the same input in the next stage. Since the tests expect a
linker error to happen, they would pass, but not for the right
reason.
Unfortunately, the current implementation uses ir_variable instances
to keep track of explicit locations. Since we don't have
ir_variables instances for individual interface members, we need
to have a custom struct with the data we need. This struct has
the ir_variable (which for interface members is the whole
interface variable), plus the data that we need to validate for
each aliased location, for now only the base type, which for
interface members we will take from the appropriate field inside
the interface variable.
Later patches will expand this custom struct so we can also check
other requirements for location aliasing, specifically that
we have matching interpolation and auxiliary storage, that once
again, we will take from the appropriate field members for the
interface variables.
Fixes:
KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations
Fixes:
tests/spec/arb_separate_shader_objects/execution/layout-location-named-block.shader_test
-auto -fbo
Fixes (these were passing before but for incorrect reasons):
tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-location-overlap.shader_test
tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-mixed-order-overlap.shader_test
---
src/compiler/glsl/link_varyings.cpp | 45 +++--
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 687bd50e52..5dc2704321 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable *var,
gl_shader_stage stage)
return var->data.location - location_start;
}
+struct explicit_location_info {
+ ir_variable *var;
+ unsigned base_type;
+};
+
static bool
-check_location_aliasing(ir_variable *explicit_locations[][4],
+check_location_aliasing(struct explicit_location_info explicit_locations[][4],
ir_variable *var,
unsigned location,
unsigned component,
@@ -427,7 +432,7 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
while (location < location_limit) {
unsigned i = component;
while (i < last_comp) {
- if (explicit_locations[location][i] != NULL) {
+ if (explicit_locations[location][i].var != NULL) {
linker_error(prog,
"%s shader has multiple outputs explicitly "
"assigned to location %d and component %d\n",
@@ -439,9 +444,9 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
/* Make sure all component at this location have the same type.
*/
for (unsigned j = 0; j < 4; j++) {
-if (explicit_locations[location][j] &&
-(explicit_locations[location][j]->type->without_array()
- ->base_type != type->without_array()->base_type)) {
+if (explicit_locations[location][j].var &&
+explicit_locations[location][j].base_type !=
+ type->without_array()->base_type) {
linker_error(prog,
"Varyings sharing the same location must "
"have the same underlying numerical type. "
@@ -450,7 +455,9 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
}
}
- explicit_locations[location][i] = var;
+ explicit_locations[location][i].var = var;
+ explicit_locations[location][i].base_type =
+type->without_array()->base_type;
i++;
/* We need to do some special handling for doubles as dvec3 and
@@ -482,8 +489,8 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx,
gl_linked_shader *consumer)
{
glsl_symbol_table parameters;
- ir_variable *explicit_locations[MAX_VARYINGS_INCL_PATCH][4] =
- { {NULL, NULL} };
+ struct explicit_location_info
explicit_locations[MAX_VARYINGS_INCL_PATCH][4] =
+ { 0 };
/* Find all shader outputs in the "producer" stage.
*/
@@ -514,9
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
Will this work with SSO shaders? Presumably the validation still has
to happen, but I don't think cross_validate_outputs_to_inputs() will
end up getting called.
On Thu, Oct 19, 2017 at 12:31 PM, Iago Toral Quiroga wrote:
> The existing code was checking the whole interface variable rather
> than its members, which is not what we want: we want to check
> aliasing for each member in the interface variable.
>
> Surprisingly, there are piglit tests that verify this and were
> passing due to a bug in the existing code: when we were computing
> the last component used by an interface variable we would use
> the 'vector' path and multiply by vector_elements, which is 0 for
> interface variables. This made the loop that checks for aliasing
> be a no-op and not add the interface variable to the list of outputs
> so then we would fail to link when we did not see a matching output
> for the same input in the next stage. Since the tests expect a
> linker error to happen, they would pass, but not for the right
> reason.
>
> Unfortunately, the current implementation uses ir_variable instances
> to keep track of explicit locations. Since we don't have
> ir_variables instances for individual interface members, we need
> to have a custom struct with the data we need. This struct has
> the ir_variable (which for interface members is the whole
> interface variable), plus the data that we need to validate for
> each aliased location, for now only the base type, which for
> interface members we will take from the appropriate field inside
> the interface variable.
>
> Later patches will expand this custom struct so we can also check
> other requirements for location aliasing, specifically that
> we have matching interpolation and auxiliary storage, that once
> again, we will take from the appropriate field members for the
> interface variables.
>
> Fixes:
> KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations
>
> Fixes:
> tests/spec/arb_separate_shader_objects/execution/layout-location-named-block.shader_test
> -auto -fbo
>
> Fixes (these were passing before but for incorrect reasons):
> tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-location-overlap.shader_test
> tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-mixed-order-overlap.shader_test
>
> ---
> src/compiler/glsl/link_varyings.cpp | 45
> +++--
> 1 file changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp
> b/src/compiler/glsl/link_varyings.cpp
> index 687bd50e52..5dc2704321 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable *var,
> gl_shader_stage stage)
> return var->data.location - location_start;
> }
>
> +struct explicit_location_info {
> + ir_variable *var;
> + unsigned base_type;
> +};
> +
> static bool
> -check_location_aliasing(ir_variable *explicit_locations[][4],
> +check_location_aliasing(struct explicit_location_info
> explicit_locations[][4],
> ir_variable *var,
> unsigned location,
> unsigned component,
> @@ -427,7 +432,7 @@ check_location_aliasing(ir_variable
> *explicit_locations[][4],
> while (location < location_limit) {
>unsigned i = component;
>while (i < last_comp) {
> - if (explicit_locations[location][i] != NULL) {
> + if (explicit_locations[location][i].var != NULL) {
> linker_error(prog,
> "%s shader has multiple outputs explicitly "
> "assigned to location %d and component %d\n",
> @@ -439,9 +444,9 @@ check_location_aliasing(ir_variable
> *explicit_locations[][4],
> /* Make sure all component at this location have the same type.
>*/
> for (unsigned j = 0; j < 4; j++) {
> -if (explicit_locations[location][j] &&
> -(explicit_locations[location][j]->type->without_array()
> - ->base_type != type->without_array()->base_type)) {
> +if (explicit_locations[location][j].var &&
> +explicit_locations[location][j].base_type !=
> + type->without_array()->base_type) {
> linker_error(prog,
> "Varyings sharing the same location must "
> "have the same underlying numerical type. "
> @@ -450,7 +455,9 @@ check_location_aliasing(ir_variable
> *explicit_locations[][4],
> }
> }
>
> - explicit_locations[location][i] = var;
> + explicit_locations[location][i].var = var;
> + explicit_locations[location][i].base_type =
> +type->without_array()->base_type;
> i++;
>
> /* We need to do some special handling for doubles as dvec3 and
> @@ -482,8 +489,8
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote:
> Will this work with SSO shaders? Presumably the validation still has
> to happen, but I don't think cross_validate_outputs_to_inputs() will
> end up getting called.
The piglit tests I mention use SSO so it seems to be working for this.
See for example:
tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-
block-member-location-overlap.shader_test
Iago
> On Thu, Oct 19, 2017 at 12:31 PM, Iago Toral Quiroga
> wrote:
> > The existing code was checking the whole interface variable rather
> > than its members, which is not what we want: we want to check
> > aliasing for each member in the interface variable.
> >
> > Surprisingly, there are piglit tests that verify this and were
> > passing due to a bug in the existing code: when we were computing
> > the last component used by an interface variable we would use
> > the 'vector' path and multiply by vector_elements, which is 0 for
> > interface variables. This made the loop that checks for aliasing
> > be a no-op and not add the interface variable to the list of
> > outputs
> > so then we would fail to link when we did not see a matching output
> > for the same input in the next stage. Since the tests expect a
> > linker error to happen, they would pass, but not for the right
> > reason.
> >
> > Unfortunately, the current implementation uses ir_variable
> > instances
> > to keep track of explicit locations. Since we don't have
> > ir_variables instances for individual interface members, we need
> > to have a custom struct with the data we need. This struct has
> > the ir_variable (which for interface members is the whole
> > interface variable), plus the data that we need to validate for
> > each aliased location, for now only the base type, which for
> > interface members we will take from the appropriate field inside
> > the interface variable.
> >
> > Later patches will expand this custom struct so we can also check
> > other requirements for location aliasing, specifically that
> > we have matching interpolation and auxiliary storage, that once
> > again, we will take from the appropriate field members for the
> > interface variables.
> >
> > Fixes:
> > KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations
> >
> > Fixes:
> > tests/spec/arb_separate_shader_objects/execution/layout-location-
> > named-block.shader_test -auto -fbo
> >
> > Fixes (these were passing before but for incorrect reasons):
> > tests/spec/arb_enhanced_layouts/linker/block-member-
> > locations/named-block-member-location-overlap.shader_test
> > tests/spec/arb_enhanced_layouts/linker/block-member-
> > locations/named-block-member-mixed-order-overlap.shader_test
> >
> > ---
> > src/compiler/glsl/link_varyings.cpp | 45
> > +++--
> > 1 file changed, 33 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 687bd50e52..5dc2704321 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable
> > *var, gl_shader_stage stage)
> > return var->data.location - location_start;
> > }
> >
> > +struct explicit_location_info {
> > + ir_variable *var;
> > + unsigned base_type;
> > +};
> > +
> > static bool
> > -check_location_aliasing(ir_variable *explicit_locations[][4],
> > +check_location_aliasing(struct explicit_location_info
> > explicit_locations[][4],
> > ir_variable *var,
> > unsigned location,
> > unsigned component,
> > @@ -427,7 +432,7 @@ check_location_aliasing(ir_variable
> > *explicit_locations[][4],
> > while (location < location_limit) {
> > unsigned i = component;
> > while (i < last_comp) {
> > - if (explicit_locations[location][i] != NULL) {
> > + if (explicit_locations[location][i].var != NULL) {
> > linker_error(prog,
> > "%s shader has multiple outputs
> > explicitly "
> > "assigned to location %d and component
> > %d\n",
> > @@ -439,9 +444,9 @@ check_location_aliasing(ir_variable
> > *explicit_locations[][4],
> > /* Make sure all component at this location have the same
> > type.
> > */
> > for (unsigned j = 0; j < 4; j++) {
> > -if (explicit_locations[location][j] &&
> > -(explicit_locations[location][j]->type-
> > >without_array()
> > - ->base_type != type->without_array()->base_type))
> > {
> > +if (explicit_locations[location][j].var &&
> > +explicit_locations[location][j].base_type !=
> > + type->without_array()->base_type) {
> > linker_error(prog,
> > "Varyings sharing the same location
> > must "
> >
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On Thu, Oct 19, 2017 at 12:40 PM, Iago Toral wrote: > On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote: >> Will this work with SSO shaders? Presumably the validation still has >> to happen, but I don't think cross_validate_outputs_to_inputs() will >> end up getting called. > > The piglit tests I mention use SSO so it seems to be working for this. > See for example: > > tests/spec/arb_enhanced_layouts/linker/block-member-locations/named- > block-member-location-overlap.shader_test Ah great. I'm a little curious how, since I don't see how cross_validate_outputs_to_inputs would get called for SSO shaders. Perhaps I'm looking at old code. Oh - because that test doesn't test SSO. It's missing a "SSO ENABLED" - can you try with that? It's just using a pipeline, but both shaders are ending up in it. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On Thu, Oct 19, 2017 at 12:45 PM, Ilia Mirkin wrote: > On Thu, Oct 19, 2017 at 12:40 PM, Iago Toral wrote: >> On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote: >>> Will this work with SSO shaders? Presumably the validation still has >>> to happen, but I don't think cross_validate_outputs_to_inputs() will >>> end up getting called. >> >> The piglit tests I mention use SSO so it seems to be working for this. >> See for example: >> >> tests/spec/arb_enhanced_layouts/linker/block-member-locations/named- >> block-member-location-overlap.shader_test > > Ah great. I'm a little curious how, since I don't see how > cross_validate_outputs_to_inputs would get called for SSO shaders. > Perhaps I'm looking at old code. > > Oh - because that test doesn't test SSO. It's missing a "SSO ENABLED" > - can you try with that? It's just using a pipeline, but both shaders > are ending up in it. BTW, my solution to all this was https://patchwork.freedesktop.org/patch/175959/ but Tim hated it, and I didn't have the time to properly respond. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On 20/10/17 04:21, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 12:45 PM, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 12:40 PM, Iago Toral wrote: On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote: Will this work with SSO shaders? Presumably the validation still has to happen, but I don't think cross_validate_outputs_to_inputs() will end up getting called. The piglit tests I mention use SSO so it seems to be working for this. See for example: tests/spec/arb_enhanced_layouts/linker/block-member-locations/named- block-member-location-overlap.shader_test Ah great. I'm a little curious how, since I don't see how cross_validate_outputs_to_inputs would get called for SSO shaders. Perhaps I'm looking at old code. Oh - because that test doesn't test SSO. It's missing a "SSO ENABLED" - can you try with that? It's just using a pipeline, but both shaders are ending up in it. BTW, my solution to all this was https://patchwork.freedesktop.org/patch/175959/ but Tim hated it, and I didn't have the time to properly respond. Hate is a strong word, the problem is it duplicated some of the checks/logic in cross_validate_outputs_to_inputs() unnecessarily. The checks should be pulled into a helper/helpers that can also be used by SSO. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On Thu, Oct 19, 2017 at 5:19 PM, Timothy Arceri wrote: > On 20/10/17 04:21, Ilia Mirkin wrote: >> >> On Thu, Oct 19, 2017 at 12:45 PM, Ilia Mirkin >> wrote: >>> >>> On Thu, Oct 19, 2017 at 12:40 PM, Iago Toral wrote: On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote: > > Will this work with SSO shaders? Presumably the validation still has > to happen, but I don't think cross_validate_outputs_to_inputs() will > end up getting called. The piglit tests I mention use SSO so it seems to be working for this. See for example: tests/spec/arb_enhanced_layouts/linker/block-member-locations/named- block-member-location-overlap.shader_test >>> >>> >>> Ah great. I'm a little curious how, since I don't see how >>> cross_validate_outputs_to_inputs would get called for SSO shaders. >>> Perhaps I'm looking at old code. >>> >>> Oh - because that test doesn't test SSO. It's missing a "SSO ENABLED" >>> - can you try with that? It's just using a pipeline, but both shaders >>> are ending up in it. >> >> >> BTW, my solution to all this was >> >> https://patchwork.freedesktop.org/patch/175959/ >> >> but Tim hated it, and I didn't have the time to properly respond. > > > Hate is a strong word, the problem is it duplicated some of the checks/logic > in cross_validate_outputs_to_inputs() unnecessarily. The checks should be > pulled into a helper/helpers that can also be used by SSO. Once I looked at the other code, I hated the duplication too - they do look sadly similar. But fixing it seemed difficult, since they were counting slightly different things, and I didn't have time to investigate in depth (and continue to not have that time). -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On 20/10/17 08:19, Timothy Arceri wrote: On 20/10/17 04:21, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 12:45 PM, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 12:40 PM, Iago Toral wrote: On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote: Will this work with SSO shaders? Presumably the validation still has to happen, but I don't think cross_validate_outputs_to_inputs() will end up getting called. The piglit tests I mention use SSO so it seems to be working for this. See for example: tests/spec/arb_enhanced_layouts/linker/block-member-locations/named- block-member-location-overlap.shader_test Ah great. I'm a little curious how, since I don't see how cross_validate_outputs_to_inputs would get called for SSO shaders. Perhaps I'm looking at old code. Oh - because that test doesn't test SSO. It's missing a "SSO ENABLED" - can you try with that? It's just using a pipeline, but both shaders are ending up in it. BTW, my solution to all this was https://patchwork.freedesktop.org/patch/175959/ but Tim hated it, and I didn't have the time to properly respond. Hate is a strong word, the problem is it duplicated some of the checks/logic in cross_validate_outputs_to_inputs() unnecessarily. The checks should be pulled into a helper/helpers that can also be used by SSO. Oh and your patch was also missing all the component checking logic which we also should be doing for SSO. Moving the checks into helpers will give us these check for free. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On 20/10/17 08:27, Timothy Arceri wrote: On 20/10/17 08:19, Timothy Arceri wrote: On 20/10/17 04:21, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 12:45 PM, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 12:40 PM, Iago Toral wrote: On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote: Will this work with SSO shaders? Presumably the validation still has to happen, but I don't think cross_validate_outputs_to_inputs() will end up getting called. The piglit tests I mention use SSO so it seems to be working for this. See for example: tests/spec/arb_enhanced_layouts/linker/block-member-locations/named- block-member-location-overlap.shader_test Ah great. I'm a little curious how, since I don't see how cross_validate_outputs_to_inputs would get called for SSO shaders. Perhaps I'm looking at old code. Oh - because that test doesn't test SSO. It's missing a "SSO ENABLED" - can you try with that? It's just using a pipeline, but both shaders are ending up in it. BTW, my solution to all this was https://patchwork.freedesktop.org/patch/175959/ but Tim hated it, and I didn't have the time to properly respond. Hate is a strong word, the problem is it duplicated some of the checks/logic in cross_validate_outputs_to_inputs() unnecessarily. The checks should be pulled into a helper/helpers that can also be used by SSO. Oh and your patch was also missing all the component checking logic which we also should be doing for SSO. Moving the checks into helpers will give us these check for free. Thinking about it some more, I don't see how your patch works for SSO. The only place you can validate the SSO pipeline is via _mesa_validate_program_pipeline() since we can't know what the other stages will be at link time. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On Thu, Oct 19, 2017 at 5:35 PM, Timothy Arceri wrote: > > > On 20/10/17 08:27, Timothy Arceri wrote: >> >> >> >> On 20/10/17 08:19, Timothy Arceri wrote: >>> >>> On 20/10/17 04:21, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 12:45 PM, Ilia Mirkin wrote: > > On Thu, Oct 19, 2017 at 12:40 PM, Iago Toral wrote: >> >> On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote: >>> >>> Will this work with SSO shaders? Presumably the validation still has >>> to happen, but I don't think cross_validate_outputs_to_inputs() will >>> end up getting called. >> >> >> The piglit tests I mention use SSO so it seems to be working for this. >> See for example: >> >> tests/spec/arb_enhanced_layouts/linker/block-member-locations/named- >> block-member-location-overlap.shader_test > > > Ah great. I'm a little curious how, since I don't see how > cross_validate_outputs_to_inputs would get called for SSO shaders. > Perhaps I'm looking at old code. > > Oh - because that test doesn't test SSO. It's missing a "SSO ENABLED" > - can you try with that? It's just using a pipeline, but both shaders > are ending up in it. BTW, my solution to all this was https://patchwork.freedesktop.org/patch/175959/ but Tim hated it, and I didn't have the time to properly respond. >>> >>> >>> Hate is a strong word, the problem is it duplicated some of the >>> checks/logic in cross_validate_outputs_to_inputs() unnecessarily. The checks >>> should be pulled into a helper/helpers that can also be used by SSO. >> >> >> Oh and your patch was also missing all the component checking logic which >> we also should be doing for SSO. Moving the checks into helpers will give us >> these check for free. > > > Thinking about it some more, I don't see how your patch works for SSO. > > The only place you can validate the SSO pipeline is via > _mesa_validate_program_pipeline() since we can't know what the other stages > will be at link time. You don't need to validate the pipeline. You need to make sure each individual program links, and the code is called at glLinkShader() time, at which point it will fail the link if the interfaces have things they're not supposed to. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On 20/10/17 08:45, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 5:35 PM, Timothy Arceri wrote: On 20/10/17 08:27, Timothy Arceri wrote: On 20/10/17 08:19, Timothy Arceri wrote: On 20/10/17 04:21, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 12:45 PM, Ilia Mirkin wrote: On Thu, Oct 19, 2017 at 12:40 PM, Iago Toral wrote: On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote: Will this work with SSO shaders? Presumably the validation still has to happen, but I don't think cross_validate_outputs_to_inputs() will end up getting called. The piglit tests I mention use SSO so it seems to be working for this. See for example: tests/spec/arb_enhanced_layouts/linker/block-member-locations/named- block-member-location-overlap.shader_test Ah great. I'm a little curious how, since I don't see how cross_validate_outputs_to_inputs would get called for SSO shaders. Perhaps I'm looking at old code. Oh - because that test doesn't test SSO. It's missing a "SSO ENABLED" - can you try with that? It's just using a pipeline, but both shaders are ending up in it. BTW, my solution to all this was https://patchwork.freedesktop.org/patch/175959/ but Tim hated it, and I didn't have the time to properly respond. Hate is a strong word, the problem is it duplicated some of the checks/logic in cross_validate_outputs_to_inputs() unnecessarily. The checks should be pulled into a helper/helpers that can also be used by SSO. Oh and your patch was also missing all the component checking logic which we also should be doing for SSO. Moving the checks into helpers will give us these check for free. Thinking about it some more, I don't see how your patch works for SSO. The only place you can validate the SSO pipeline is via _mesa_validate_program_pipeline() since we can't know what the other stages will be at link time. You don't need to validate the pipeline. You need to make sure each individual program links, and the code is called at glLinkShader() time, at which point it will fail the link if the interfaces have things they're not supposed to. Oh right, this is just for validating the outward facing interfaces of a SSO program. I think we have discussed this before :P ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On 20/10/17 03:31, Iago Toral Quiroga wrote:
The existing code was checking the whole interface variable rather
than its members, which is not what we want: we want to check
aliasing for each member in the interface variable.
Surprisingly, there are piglit tests that verify this and were
passing due to a bug in the existing code: when we were computing
the last component used by an interface variable we would use
the 'vector' path and multiply by vector_elements, which is 0 for
interface variables. This made the loop that checks for aliasing
be a no-op and not add the interface variable to the list of outputs
so then we would fail to link when we did not see a matching output
for the same input in the next stage. Since the tests expect a
linker error to happen, they would pass, but not for the right
reason.
Unfortunately, the current implementation uses ir_variable instances
to keep track of explicit locations. Since we don't have
ir_variables instances for individual interface members, we need
to have a custom struct with the data we need. This struct has
the ir_variable (which for interface members is the whole
interface variable), plus the data that we need to validate for
each aliased location, for now only the base type, which for
interface members we will take from the appropriate field inside
the interface variable.
Later patches will expand this custom struct so we can also check
other requirements for location aliasing, specifically that
we have matching interpolation and auxiliary storage, that once
again, we will take from the appropriate field members for the
interface variables.
Fixes:
KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations
Fixes:
tests/spec/arb_separate_shader_objects/execution/layout-location-named-block.shader_test
-auto -fbo
Fixes (these were passing before but for incorrect reasons):
tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-location-overlap.shader_test
tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-mixed-order-overlap.shader_test
---
src/compiler/glsl/link_varyings.cpp | 45 +++--
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 687bd50e52..5dc2704321 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable *var,
gl_shader_stage stage)
return var->data.location - location_start;
}
+struct explicit_location_info {
+ ir_variable *var;
+ unsigned base_type;
+};
+
static bool
-check_location_aliasing(ir_variable *explicit_locations[][4],
+check_location_aliasing(struct explicit_location_info explicit_locations[][4],
ir_variable *var,
unsigned location,
unsigned component,
@@ -427,7 +432,7 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
while (location < location_limit) {
unsigned i = component;
while (i < last_comp) {
- if (explicit_locations[location][i] != NULL) {
+ if (explicit_locations[location][i].var != NULL) {
linker_error(prog,
"%s shader has multiple outputs explicitly "
"assigned to location %d and component %d\n",
@@ -439,9 +444,9 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
/* Make sure all component at this location have the same type.
*/
for (unsigned j = 0; j < 4; j++) {
-if (explicit_locations[location][j] &&
-(explicit_locations[location][j]->type->without_array()
- ->base_type != type->without_array()->base_type)) {
+if (explicit_locations[location][j].var &&
+explicit_locations[location][j].base_type !=
+ type->without_array()->base_type) {
linker_error(prog,
"Varyings sharing the same location must "
"have the same underlying numerical type. "
@@ -450,7 +455,9 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
}
}
- explicit_locations[location][i] = var;
+ explicit_locations[location][i].var = var;
+ explicit_locations[location][i].base_type =
+type->without_array()->base_type;
i++;
/* We need to do some special handling for doubles as dvec3 and
@@ -482,8 +489,8 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx,
gl_linked_shader *consumer)
{
glsl_symbol_table parameters;
- ir_variable *explicit_locations[MAX_VARYINGS_INCL_PATCH][4] =
- { {NULL, NULL} };
+ struct explicit_location_info
explicit_locations[MAX_VARYINGS_INCL_PATCH][4] =
+
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On Thu, Oct 19, 2017 at 10:18 PM, Timothy Arceri wrote:
>
>
> On 20/10/17 03:31, Iago Toral Quiroga wrote:
>>
>> The existing code was checking the whole interface variable rather
>> than its members, which is not what we want: we want to check
>> aliasing for each member in the interface variable.
>>
>> Surprisingly, there are piglit tests that verify this and were
>> passing due to a bug in the existing code: when we were computing
>> the last component used by an interface variable we would use
>> the 'vector' path and multiply by vector_elements, which is 0 for
>> interface variables. This made the loop that checks for aliasing
>> be a no-op and not add the interface variable to the list of outputs
>> so then we would fail to link when we did not see a matching output
>> for the same input in the next stage. Since the tests expect a
>> linker error to happen, they would pass, but not for the right
>> reason.
>>
>> Unfortunately, the current implementation uses ir_variable instances
>> to keep track of explicit locations. Since we don't have
>> ir_variables instances for individual interface members, we need
>> to have a custom struct with the data we need. This struct has
>> the ir_variable (which for interface members is the whole
>> interface variable), plus the data that we need to validate for
>> each aliased location, for now only the base type, which for
>> interface members we will take from the appropriate field inside
>> the interface variable.
>>
>> Later patches will expand this custom struct so we can also check
>> other requirements for location aliasing, specifically that
>> we have matching interpolation and auxiliary storage, that once
>> again, we will take from the appropriate field members for the
>> interface variables.
>>
>> Fixes:
>> KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations
>>
>> Fixes:
>>
>> tests/spec/arb_separate_shader_objects/execution/layout-location-named-block.shader_test
>> -auto -fbo
>>
>> Fixes (these were passing before but for incorrect reasons):
>>
>> tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-location-overlap.shader_test
>>
>> tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-mixed-order-overlap.shader_test
>>
>> ---
>> src/compiler/glsl/link_varyings.cpp | 45
>> +++--
>> 1 file changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/compiler/glsl/link_varyings.cpp
>> b/src/compiler/glsl/link_varyings.cpp
>> index 687bd50e52..5dc2704321 100644
>> --- a/src/compiler/glsl/link_varyings.cpp
>> +++ b/src/compiler/glsl/link_varyings.cpp
>> @@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable *var,
>> gl_shader_stage stage)
>> return var->data.location - location_start;
>> }
>> +struct explicit_location_info {
>> + ir_variable *var;
>> + unsigned base_type;
>> +};
>> +
>> static bool
>> -check_location_aliasing(ir_variable *explicit_locations[][4],
>> +check_location_aliasing(struct explicit_location_info
>> explicit_locations[][4],
>> ir_variable *var,
>> unsigned location,
>> unsigned component,
>> @@ -427,7 +432,7 @@ check_location_aliasing(ir_variable
>> *explicit_locations[][4],
>> while (location < location_limit) {
>> unsigned i = component;
>> while (i < last_comp) {
>> - if (explicit_locations[location][i] != NULL) {
>> + if (explicit_locations[location][i].var != NULL) {
>> linker_error(prog,
>>"%s shader has multiple outputs explicitly "
>>"assigned to location %d and component %d\n",
>> @@ -439,9 +444,9 @@ check_location_aliasing(ir_variable
>> *explicit_locations[][4],
>>/* Make sure all component at this location have the same type.
>> */
>>for (unsigned j = 0; j < 4; j++) {
>> -if (explicit_locations[location][j] &&
>> -(explicit_locations[location][j]->type->without_array()
>> - ->base_type != type->without_array()->base_type)) {
>> +if (explicit_locations[location][j].var &&
>> +explicit_locations[location][j].base_type !=
>> + type->without_array()->base_type) {
>> linker_error(prog,
>> "Varyings sharing the same location must "
>> "have the same underlying numerical type. "
>> @@ -450,7 +455,9 @@ check_location_aliasing(ir_variable
>> *explicit_locations[][4],
>> }
>>}
>> - explicit_locations[location][i] = var;
>> + explicit_locations[location][i].var = var;
>> + explicit_locations[location][i].base_type =
>> +type->without_array()->base_type;
>>i++;
>> /* We need to do some special handling for doubles as dvec3
>> and
>
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On 20/10/17 13:36, Ilia Mirkin wrote:
On Thu, Oct 19, 2017 at 10:18 PM, Timothy Arceri wrote:
On 20/10/17 03:31, Iago Toral Quiroga wrote:
The existing code was checking the whole interface variable rather
than its members, which is not what we want: we want to check
aliasing for each member in the interface variable.
Surprisingly, there are piglit tests that verify this and were
passing due to a bug in the existing code: when we were computing
the last component used by an interface variable we would use
the 'vector' path and multiply by vector_elements, which is 0 for
interface variables. This made the loop that checks for aliasing
be a no-op and not add the interface variable to the list of outputs
so then we would fail to link when we did not see a matching output
for the same input in the next stage. Since the tests expect a
linker error to happen, they would pass, but not for the right
reason.
Unfortunately, the current implementation uses ir_variable instances
to keep track of explicit locations. Since we don't have
ir_variables instances for individual interface members, we need
to have a custom struct with the data we need. This struct has
the ir_variable (which for interface members is the whole
interface variable), plus the data that we need to validate for
each aliased location, for now only the base type, which for
interface members we will take from the appropriate field inside
the interface variable.
Later patches will expand this custom struct so we can also check
other requirements for location aliasing, specifically that
we have matching interpolation and auxiliary storage, that once
again, we will take from the appropriate field members for the
interface variables.
Fixes:
KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations
Fixes:
tests/spec/arb_separate_shader_objects/execution/layout-location-named-block.shader_test
-auto -fbo
Fixes (these were passing before but for incorrect reasons):
tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-location-overlap.shader_test
tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-mixed-order-overlap.shader_test
---
src/compiler/glsl/link_varyings.cpp | 45
+++--
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 687bd50e52..5dc2704321 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable *var,
gl_shader_stage stage)
return var->data.location - location_start;
}
+struct explicit_location_info {
+ ir_variable *var;
+ unsigned base_type;
+};
+
static bool
-check_location_aliasing(ir_variable *explicit_locations[][4],
+check_location_aliasing(struct explicit_location_info
explicit_locations[][4],
ir_variable *var,
unsigned location,
unsigned component,
@@ -427,7 +432,7 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
while (location < location_limit) {
unsigned i = component;
while (i < last_comp) {
- if (explicit_locations[location][i] != NULL) {
+ if (explicit_locations[location][i].var != NULL) {
linker_error(prog,
"%s shader has multiple outputs explicitly "
"assigned to location %d and component %d\n",
@@ -439,9 +444,9 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
/* Make sure all component at this location have the same type.
*/
for (unsigned j = 0; j < 4; j++) {
-if (explicit_locations[location][j] &&
-(explicit_locations[location][j]->type->without_array()
- ->base_type != type->without_array()->base_type)) {
+if (explicit_locations[location][j].var &&
+explicit_locations[location][j].base_type !=
+ type->without_array()->base_type) {
linker_error(prog,
"Varyings sharing the same location must "
"have the same underlying numerical type. "
@@ -450,7 +455,9 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
}
}
- explicit_locations[location][i] = var;
+ explicit_locations[location][i].var = var;
+ explicit_locations[location][i].base_type =
+type->without_array()->base_type;
i++;
/* We need to do some special handling for doubles as dvec3
and
@@ -482,8 +489,8 @@ cross_validate_outputs_to_inputs(struct gl_context
*ctx,
gl_linked_shader *consumer)
{
glsl_symbol_table parameters;
- ir_variable *explicit_locations[MAX_VARYINGS_INCL_PATCH][4
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On Fri, 2017-10-20 at 13:18 +1100, Timothy Arceri wrote:
>
> On 20/10/17 03:31, Iago Toral Quiroga wrote:
> > The existing code was checking the whole interface variable rather
> > than its members, which is not what we want: we want to check
> > aliasing for each member in the interface variable.
> >
> > Surprisingly, there are piglit tests that verify this and were
> > passing due to a bug in the existing code: when we were computing
> > the last component used by an interface variable we would use
> > the 'vector' path and multiply by vector_elements, which is 0 for
> > interface variables. This made the loop that checks for aliasing
> > be a no-op and not add the interface variable to the list of
> > outputs
> > so then we would fail to link when we did not see a matching output
> > for the same input in the next stage. Since the tests expect a
> > linker error to happen, they would pass, but not for the right
> > reason.
> >
> > Unfortunately, the current implementation uses ir_variable
> > instances
> > to keep track of explicit locations. Since we don't have
> > ir_variables instances for individual interface members, we need
> > to have a custom struct with the data we need. This struct has
> > the ir_variable (which for interface members is the whole
> > interface variable), plus the data that we need to validate for
> > each aliased location, for now only the base type, which for
> > interface members we will take from the appropriate field inside
> > the interface variable.
> >
> > Later patches will expand this custom struct so we can also check
> > other requirements for location aliasing, specifically that
> > we have matching interpolation and auxiliary storage, that once
> > again, we will take from the appropriate field members for the
> > interface variables.
> >
> > Fixes:
> > KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations
> >
> > Fixes:
> > tests/spec/arb_separate_shader_objects/execution/layout-location-
> > named-block.shader_test -auto -fbo
> >
> > Fixes (these were passing before but for incorrect reasons):
> > tests/spec/arb_enhanced_layouts/linker/block-member-
> > locations/named-block-member-location-overlap.shader_test
> > tests/spec/arb_enhanced_layouts/linker/block-member-
> > locations/named-block-member-mixed-order-overlap.shader_test
> >
> > ---
> > src/compiler/glsl/link_varyings.cpp | 45
> > +++--
> > 1 file changed, 33 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 687bd50e52..5dc2704321 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable
> > *var, gl_shader_stage stage)
> > return var->data.location - location_start;
> > }
> >
> > +struct explicit_location_info {
> > + ir_variable *var;
> > + unsigned base_type;
> > +};
> > +
> > static bool
> > -check_location_aliasing(ir_variable *explicit_locations[][4],
> > +check_location_aliasing(struct explicit_location_info
> > explicit_locations[][4],
> > ir_variable *var,
> > unsigned location,
> > unsigned component,
> > @@ -427,7 +432,7 @@ check_location_aliasing(ir_variable
> > *explicit_locations[][4],
> > while (location < location_limit) {
> > unsigned i = component;
> > while (i < last_comp) {
> > - if (explicit_locations[location][i] != NULL) {
> > + if (explicit_locations[location][i].var != NULL) {
> > linker_error(prog,
> > "%s shader has multiple outputs
> > explicitly "
> > "assigned to location %d and component
> > %d\n",
> > @@ -439,9 +444,9 @@ check_location_aliasing(ir_variable
> > *explicit_locations[][4],
> > /* Make sure all component at this location have the
> > same type.
> > */
> > for (unsigned j = 0; j < 4; j++) {
> > -if (explicit_locations[location][j] &&
> > -(explicit_locations[location][j]->type-
> > >without_array()
> > - ->base_type != type->without_array()->base_type))
> > {
> > +if (explicit_locations[location][j].var &&
> > +explicit_locations[location][j].base_type !=
> > + type->without_array()->base_type) {
> > linker_error(prog,
> > "Varyings sharing the same location
> > must "
> > "have the same underlying numerical
> > type. "
> > @@ -450,7 +455,9 @@ check_location_aliasing(ir_variable
> > *explicit_locations[][4],
> > }
> > }
> >
> > - explicit_locations[location][i] = var;
> > + explicit_locations[location][i].var = var;
> > + explicit_locations[location][i].base_type =
Re: [Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
On 20/10/17 16:54, Iago Toral wrote:
On Fri, 2017-10-20 at 13:18 +1100, Timothy Arceri wrote:
On 20/10/17 03:31, Iago Toral Quiroga wrote:
The existing code was checking the whole interface variable rather
than its members, which is not what we want: we want to check
aliasing for each member in the interface variable.
Surprisingly, there are piglit tests that verify this and were
passing due to a bug in the existing code: when we were computing
the last component used by an interface variable we would use
the 'vector' path and multiply by vector_elements, which is 0 for
interface variables. This made the loop that checks for aliasing
be a no-op and not add the interface variable to the list of
outputs
so then we would fail to link when we did not see a matching output
for the same input in the next stage. Since the tests expect a
linker error to happen, they would pass, but not for the right
reason.
Unfortunately, the current implementation uses ir_variable
instances
to keep track of explicit locations. Since we don't have
ir_variables instances for individual interface members, we need
to have a custom struct with the data we need. This struct has
the ir_variable (which for interface members is the whole
interface variable), plus the data that we need to validate for
each aliased location, for now only the base type, which for
interface members we will take from the appropriate field inside
the interface variable.
Later patches will expand this custom struct so we can also check
other requirements for location aliasing, specifically that
we have matching interpolation and auxiliary storage, that once
again, we will take from the appropriate field members for the
interface variables.
Fixes:
KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations
Fixes:
tests/spec/arb_separate_shader_objects/execution/layout-location-
named-block.shader_test -auto -fbo
Fixes (these were passing before but for incorrect reasons):
tests/spec/arb_enhanced_layouts/linker/block-member-
locations/named-block-member-location-overlap.shader_test
tests/spec/arb_enhanced_layouts/linker/block-member-
locations/named-block-member-mixed-order-overlap.shader_test
---
src/compiler/glsl/link_varyings.cpp | 45
+++--
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 687bd50e52..5dc2704321 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable
*var, gl_shader_stage stage)
return var->data.location - location_start;
}
+struct explicit_location_info {
+ ir_variable *var;
+ unsigned base_type;
+};
+
static bool
-check_location_aliasing(ir_variable *explicit_locations[][4],
+check_location_aliasing(struct explicit_location_info
explicit_locations[][4],
ir_variable *var,
unsigned location,
unsigned component,
@@ -427,7 +432,7 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
while (location < location_limit) {
unsigned i = component;
while (i < last_comp) {
- if (explicit_locations[location][i] != NULL) {
+ if (explicit_locations[location][i].var != NULL) {
linker_error(prog,
"%s shader has multiple outputs
explicitly "
"assigned to location %d and component
%d\n",
@@ -439,9 +444,9 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
/* Make sure all component at this location have the
same type.
*/
for (unsigned j = 0; j < 4; j++) {
-if (explicit_locations[location][j] &&
-(explicit_locations[location][j]->type-
without_array()
- ->base_type != type->without_array()->base_type))
{
+if (explicit_locations[location][j].var &&
+explicit_locations[location][j].base_type !=
+ type->without_array()->base_type) {
linker_error(prog,
"Varyings sharing the same location
must "
"have the same underlying numerical
type. "
@@ -450,7 +455,9 @@ check_location_aliasing(ir_variable
*explicit_locations[][4],
}
}
- explicit_locations[location][i] = var;
+ explicit_locations[location][i].var = var;
+ explicit_locations[location][i].base_type =
+type->without_array()->base_type;
i++;
/* We need to do some special handling for doubles as
dvec3 and
@@ -482,8 +489,8 @@ cross_validate_outputs_to_inputs(struct
gl_context *ctx,
gl_linked_shader *consumer)
{
glsl_symbol_table parameters;
- ir_variable *explicit_locations[MAX_VARYINGS_INCL_PA
