[Mesa-dev] [PATCH 19/28] glsl: add support for explicit components to frag outputs

[Timothy Arceri]

On Tue, 2016-01-12 at 16:36 -0800, Anuj Phogat wrote:
> On Mon, Dec 28, 2015 at 9:00 PM, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > ---
> >  src/glsl/linker.cpp | 56
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 55 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > index 41ff057..44dd7f0 100644
> > --- a/src/glsl/linker.cpp
> > +++ b/src/glsl/linker.cpp
> > @@ -2411,7 +2411,12 @@
> > assign_attribute_or_color_locations(gl_shader_program *prog,
> >        }
> >     } to_assign[16];
> > 
> > +   /* Temporary array for the set of attributes that have
> > locations assigned.
> > +    */
> > +   ir_variable *assigned[16];
> > +
> >     unsigned num_attr = 0;
> > +   unsigned assigned_attr = 0;
> > 
> >     foreach_in_list(ir_instruction, node, sh->ir) {
> >        ir_variable *const var = node->as_variable();
> > @@ -2573,7 +2578,53 @@
> > assign_attribute_or_color_locations(gl_shader_program *prog,
> >              * attribute overlaps any previously allocated bits.
> >              */
> >             if ((~(use_mask << attr) & used_locations) !=
> > used_locations) {
> > -               if (target_index == MESA_SHADER_FRAGMENT ||
> > +               if (target_index == MESA_SHADER_FRAGMENT && !prog
> > ->IsES) {
> > +                  /* From section 4.4.2 (Output Layout Qualifiers)
> > of the GLSL
> > +                   * 4.40 spec:
> > +                   *
> > +                   *    "Additionally, for fragment shader
> > outputs, if two
> > +                   *    variables are placed within the same
> > location, they
> > +                   *    must have the same underlying type
> > (floating-point or
> > +                   *    integer). No component aliasing of output
> > variables or
> > +                   *    members is allowed.
> > +                   */
> > +                  int frag_out_end_loc = (var->type->is_array() ?
> > +                     var->type->arrays_of_arrays_size() : 1) +
> > +                     var->data.location;
> > +
> > +                  for (unsigned i = 0; i < assigned_attr; i++) {
> > +                     for (int j = var->data.location; j <
> > frag_out_end_loc;
> > +                          j++) {
> > +                        if (assigned[i]->data.location == j) {
> I find assigned[i]->data.location == var->data.location more
> readable.

This comment got me looking at this code and the piglit tests again and
I seem to be missing a piglit test for overlaping array output from the
fragment shader.

... 20 minute later after writing the tests, it seems that both error
checks for arrays and components are only half working correctly with
this patch.

I'm about to send a V2 and have already sent the piglit tests:

http://patchwork.freedesktop.org/patch/70351/

> 
> > +                           if (assigned[i]->type->without_array()
> > ->base_type !=
> > +                               var->type->without_array()
> > ->base_type) {
> > +                              linker_error(prog,
> > +                                           "types do not match for
> > aliased"
> > +                                           " %ss %s and %s\n",
> > string,
> > +                                           assigned[i]->name, var
> > ->name);
> > +                              return false;
> > +                           }
> > +
> > +                           if ((assigned[i]->data.location_frac ==
> > +                                var->data.location_frac) ||
> > +                              ((assigned[i]->data.location_frac <
> > +                                var->data.location_frac) &&
> > +                                ((assigned[i]->data.location_frac
> > +
> > +                                  assigned[i]->type
> > ->vector_elements) >
> > +                                 var->data.location_frac))) {
> > +                              linker_error(prog,
> > +                                           "overlapping component
> > is "
> > +                                           "assigned to %ss %s and
> > %s "
> > +                                           "(component=%d)\n",
> > +                                           string, assigned[i]
> > ->name,
> > +                                           var->name,
> > +                                           var
> > ->data.location_frac);
> > +                              return false;
> > +                           }
> > +                        }
> > +                     }
> > +                  }
> > +               } else if (target_index == MESA_SHADER_FRAGMENT ||
> >                     (prog->IsES && prog->Version >= 300)) {
> >                    linker_error(prog,
> >                                 "overlapping location is assigned "
> > @@ -2614,6 +2665,9 @@
> > assign_attribute_or_color_locations(gl_shader_program *prog,
> >                 double_storage_locations |= (use_mask << attr);
> >          }
> > 
> > +         assigned[assigned_attr] = var;
> > +         assigned_attr++;
> > +
> >          continue;
> >        }
> > 
> > --
> > 2.4.3
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev