Re: [Mesa-dev] [PATCH v2 4/8] glsl/linker: outputs in the same location must share interpolation

[Iago Toral]

On Tue, 2017-10-24 at 23:45 -0400, Ilia Mirkin wrote:
> On Tue, Oct 24, 2017 at 5:28 AM, Iago Toral Quiroga  m> wrote:
> > From ARB_enhanced_layouts:
> > 
> > "[...]when location aliasing, the aliases sharing the location
> >  must have the same underlying numerical type (floating-point or
> >  integer) and the same auxiliary storage and
> >  interpolation qualification.[...]"
> > 
> > Add code to the linker to validate that aliased locations do
> > have the same interpolation.
> > 
> > Fixes:
> > KHR-
> > GL45.enhanced_layouts.varying_location_aliasing_with_mixed_interpol
> > ation
> > 
> > Reviewed-by: Timothy Arceri 
> > ---
> >  src/compiler/glsl/link_varyings.cpp | 45
> > +
> >  1 file changed, 41 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 1db851b7e9..9542754600 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -406,6 +406,7 @@ compute_variable_location_slot(ir_variable
> > *var, gl_shader_stage stage)
> >  struct explicit_location_info {
> > ir_variable *var;
> > unsigned base_type;
> > +   unsigned interpolation;
> >  };
> > 
> >  static bool
> > @@ -415,6 +416,7 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> >  unsigned component,
> >  unsigned location_limit,
> >  const glsl_type *type,
> > +unsigned interpolation,
> >  gl_shader_program *prog,
> >  gl_shader_stage stage)
> >  {
> > @@ -431,6 +433,38 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> > 
> > while (location < location_limit) {
> >    unsigned i = component;
> > +
> > +  /* If there are other outputs assigned to the same location
> > +   * they must have the same interpolation
> > +   */
> > +  unsigned comp = 0;
> > +  while (comp < 4) {
> > + /* Skip the components used by this output, we only care
> > about
> > + * other outputs in the same location
> 
> IMHO this should get combined with the type compatibility check --
> it's doing fundamentally the same thing.

Yeah, that makes sense. Thanks!

> > +  */
> > + if (comp == i) {
> > +comp = last_comp;
> > +continue;
> > + }
> > +
> > + struct explicit_location_info *info =
> > +_locations[location][comp];
> > +
> > + if (info->var) {
> > +if (info->interpolation != interpolation) {
> > +   linker_error(prog,
> > +"%s shader has multiple outputs at
> > explicit "
> > +"location %u with different
> > interpolation "
> > +"settings\n",
> > +_mesa_shader_stage_to_string(stage),
> > location);
> > +   return false;
> > +}
> > + }
> > +
> > + comp++;
> > +  }
> > +
> > +  /* Component aliasing is not allowed */
> >    while (i < last_comp) {
> >   if (explicit_locations[location][i].var != NULL) {
> >  linker_error(prog,
> > @@ -458,6 +492,7 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> >   explicit_locations[location][i].var = var;
> >   explicit_locations[location][i].base_type =
> >  type->without_array()->base_type;
> > + explicit_locations[location][i].interpolation =
> > interpolation;
> >   i++;
> > 
> >   /* We need to do some special handling for doubles as
> > dvec3 and
> > @@ -526,18 +561,20 @@ cross_validate_outputs_to_inputs(struct
> > gl_context *ctx,
> > unsigned field_location = type-
> > >fields.structure[i].location -
> >    (type->fields.structure[i].patch ?
> > VARYING_SLOT_PATCH0 :
> >   VARYING_SLOT_
> > VAR0);
> > +   unsigned interpolation = type-
> > >fields.structure[i].interpolation;
> > if (!check_location_aliasing(explicit_locations,
> > var,
> >  field_location,
> >  0, field_location + 1,
> > -field_type, prog,
> > -producer->Stage)) {
> > +field_type,
> > interpolation,
> > +prog, producer-
> > >Stage)) {
> >    return;
> > }
> >  }
> >   } else if (!check_location_aliasing(explicit_locations,
> > var,
> >   idx, var-
> > 

Re: [Mesa-dev] [PATCH v2 4/8] glsl/linker: outputs in the same location must share interpolation

[Ilia Mirkin]

On Tue, Oct 24, 2017 at 5:28 AM, Iago Toral Quiroga  wrote:
> From ARB_enhanced_layouts:
>
> "[...]when location aliasing, the aliases sharing the location
>  must have the same underlying numerical type (floating-point or
>  integer) and the same auxiliary storage and
>  interpolation qualification.[...]"
>
> Add code to the linker to validate that aliased locations do
> have the same interpolation.
>
> Fixes:
> KHR-GL45.enhanced_layouts.varying_location_aliasing_with_mixed_interpolation
>
> Reviewed-by: Timothy Arceri 
> ---
>  src/compiler/glsl/link_varyings.cpp | 45 
> +
>  1 file changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp 
> b/src/compiler/glsl/link_varyings.cpp
> index 1db851b7e9..9542754600 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -406,6 +406,7 @@ compute_variable_location_slot(ir_variable *var, 
> gl_shader_stage stage)
>  struct explicit_location_info {
> ir_variable *var;
> unsigned base_type;
> +   unsigned interpolation;
>  };
>
>  static bool
> @@ -415,6 +416,7 @@ check_location_aliasing(struct explicit_location_info 
> explicit_locations[][4],
>  unsigned component,
>  unsigned location_limit,
>  const glsl_type *type,
> +unsigned interpolation,
>  gl_shader_program *prog,
>  gl_shader_stage stage)
>  {
> @@ -431,6 +433,38 @@ check_location_aliasing(struct explicit_location_info 
> explicit_locations[][4],
>
> while (location < location_limit) {
>unsigned i = component;
> +
> +  /* If there are other outputs assigned to the same location
> +   * they must have the same interpolation
> +   */
> +  unsigned comp = 0;
> +  while (comp < 4) {
> + /* Skip the components used by this output, we only care about
> + * other outputs in the same location

IMHO this should get combined with the type compatibility check --
it's doing fundamentally the same thing.

> +  */
> + if (comp == i) {
> +comp = last_comp;
> +continue;
> + }
> +
> + struct explicit_location_info *info =
> +_locations[location][comp];
> +
> + if (info->var) {
> +if (info->interpolation != interpolation) {
> +   linker_error(prog,
> +"%s shader has multiple outputs at explicit "
> +"location %u with different interpolation "
> +"settings\n",
> +_mesa_shader_stage_to_string(stage), location);
> +   return false;
> +}
> + }
> +
> + comp++;
> +  }
> +
> +  /* Component aliasing is not allowed */
>while (i < last_comp) {
>   if (explicit_locations[location][i].var != NULL) {
>  linker_error(prog,
> @@ -458,6 +492,7 @@ check_location_aliasing(struct explicit_location_info 
> explicit_locations[][4],
>   explicit_locations[location][i].var = var;
>   explicit_locations[location][i].base_type =
>  type->without_array()->base_type;
> + explicit_locations[location][i].interpolation = interpolation;
>   i++;
>
>   /* We need to do some special handling for doubles as dvec3 and
> @@ -526,18 +561,20 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx,
> unsigned field_location = type->fields.structure[i].location -
>(type->fields.structure[i].patch ? VARYING_SLOT_PATCH0 :
>   VARYING_SLOT_VAR0);
> +   unsigned interpolation = 
> type->fields.structure[i].interpolation;
> if (!check_location_aliasing(explicit_locations, var,
>  field_location,
>  0, field_location + 1,
> -field_type, prog,
> -producer->Stage)) {
> +field_type, interpolation,
> +prog, producer->Stage)) {
>return;
> }
>  }
>   } else if (!check_location_aliasing(explicit_locations, var,
>   idx, var->data.location_frac,
> - slot_limit, type, prog,
> - producer->Stage)) {
> + slot_limit, type,
> + var->data.interpolation,
> + prog, producer->Stage)) {
>  

[Mesa-dev] [PATCH v2 4/8] glsl/linker: outputs in the same location must share interpolation

[Iago Toral Quiroga]

From ARB_enhanced_layouts:

"[...]when location aliasing, the aliases sharing the location
 must have the same underlying numerical type (floating-point or
 integer) and the same auxiliary storage and
 interpolation qualification.[...]"

Add code to the linker to validate that aliased locations do
have the same interpolation.

Fixes:
KHR-GL45.enhanced_layouts.varying_location_aliasing_with_mixed_interpolation

Reviewed-by: Timothy Arceri 
---
 src/compiler/glsl/link_varyings.cpp | 45 +
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/src/compiler/glsl/link_varyings.cpp 
b/src/compiler/glsl/link_varyings.cpp
index 1db851b7e9..9542754600 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -406,6 +406,7 @@ compute_variable_location_slot(ir_variable *var, 
gl_shader_stage stage)
 struct explicit_location_info {
ir_variable *var;
unsigned base_type;
+   unsigned interpolation;
 };
 
 static bool
@@ -415,6 +416,7 @@ check_location_aliasing(struct explicit_location_info 
explicit_locations[][4],
 unsigned component,
 unsigned location_limit,
 const glsl_type *type,
+unsigned interpolation,
 gl_shader_program *prog,
 gl_shader_stage stage)
 {
@@ -431,6 +433,38 @@ check_location_aliasing(struct explicit_location_info 
explicit_locations[][4],
 
while (location < location_limit) {
   unsigned i = component;
+
+  /* If there are other outputs assigned to the same location
+   * they must have the same interpolation
+   */
+  unsigned comp = 0;
+  while (comp < 4) {
+ /* Skip the components used by this output, we only care about
+ * other outputs in the same location
+  */
+ if (comp == i) {
+comp = last_comp;
+continue;
+ }
+
+ struct explicit_location_info *info =
+_locations[location][comp];
+
+ if (info->var) {
+if (info->interpolation != interpolation) {
+   linker_error(prog,
+"%s shader has multiple outputs at explicit "
+"location %u with different interpolation "
+"settings\n",
+_mesa_shader_stage_to_string(stage), location);
+   return false;
+}
+ }
+
+ comp++;
+  }
+
+  /* Component aliasing is not allowed */
   while (i < last_comp) {
  if (explicit_locations[location][i].var != NULL) {
 linker_error(prog,
@@ -458,6 +492,7 @@ check_location_aliasing(struct explicit_location_info 
explicit_locations[][4],
  explicit_locations[location][i].var = var;
  explicit_locations[location][i].base_type =
 type->without_array()->base_type;
+ explicit_locations[location][i].interpolation = interpolation;
  i++;
 
  /* We need to do some special handling for doubles as dvec3 and
@@ -526,18 +561,20 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx,
unsigned field_location = type->fields.structure[i].location -
   (type->fields.structure[i].patch ? VARYING_SLOT_PATCH0 :
  VARYING_SLOT_VAR0);
+   unsigned interpolation = 
type->fields.structure[i].interpolation;
if (!check_location_aliasing(explicit_locations, var,
 field_location,
 0, field_location + 1,
-field_type, prog,
-producer->Stage)) {
+field_type, interpolation,
+prog, producer->Stage)) {
   return;
}
 }
  } else if (!check_location_aliasing(explicit_locations, var,
  idx, var->data.location_frac,
- slot_limit, type, prog,
- producer->Stage)) {
+ slot_limit, type,
+ var->data.interpolation,
+ prog, producer->Stage)) {
 return;
  }
   }
-- 
2.11.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev