[Mesa-dev] [PATCH v2] glsl: Fix aggregates with dynamic initializers.

[Cody Northrop]

No, that's the same experiment I would have run.  I was hoping the original
authors could chime in, but it sounds like you have more insight via AoA
work.

I'll resubmit with your change, thanks!

-C



On Wed, Jul 9, 2014 at 5:36 PM, Timothy Arceri <t_arceri at yahoo.com.au>
wrote:

> On Wed, 2014-07-02 at 22:16 +1000, Timothy Arceri wrote:
> > On Tue, 2014-07-01 at 14:45 -0700, Kenneth Graunke wrote:
> > > From: Cody Northrop <cody at lunarg.com>
> > >
> > > Vectors are falling in to the ir_dereference_array() path.
> > >
> > > Without this change, the following glsl aborts the debug driver,
> > > or gets the wrong answer in release:
> > >
> > > mat2x2 a = mat2( vec2( 1.0, vertex.x ), vec2( 0.0, 1.0 ) );
> > >
> > > Also submitting piglit tests, will reference in bug.
> > >
> > > v2: Rebase on Mesa master.
> > >
> > > Signed-off-by: Cody Northrop <cody at lunarg.com>
> > > Reviewed-by: Courtney Goeltzenleuchter <courtney at lunarg.com>
> > > Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79373
> > > ---
> > >  src/glsl/ast_function.cpp | 16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > Hi Cody,
> > >
> > > Your patch didn't apply to master due to Matt's foreach_list changes;
> > > I did the obvious rebase.  Otherwise, it looks great.  Thanks for
> fixing
> > > this and improving our tests!
> > >
> > > I'll plan to commit this tomorrow unless anyone else has objections.
> > >
> > >  --Ken
> > >
> > > diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> > > index cdb34cc..98288d2 100644
> > > --- a/src/glsl/ast_function.cpp
> > > +++ b/src/glsl/ast_function.cpp
> > > @@ -743,10 +743,20 @@ process_vec_mat_constructor(exec_list
> *instructions,
> > >
> > >     int i = 0;
> > >     foreach_in_list(ir_rvalue, rhs, &actual_parameters) {
> > > -      ir_rvalue *lhs = new(ctx) ir_dereference_array(var,
> > > -                                                     new(ctx)
> ir_constant(i));
> > > +      ir_instruction *assignment = NULL;
> > > +
> > > +      if (var->type->is_array() || var->type->is_matrix()) {
> >
> > Is this ever actually an array???
>
> I didn't mean for my question to hold this up for so long. But I cant
> see where this would ever be an array, if my understanding is correct
> process_array_constructor() should process any arrays. When removing
> var->type->is_array() all arb_shading_language_420pack piglit tests
> (including the new ones) continue to pass. So I don't think the
> is_array() condition is needed.
>
> If I'm wrong could you please give an example of when this would be an
> array as it might affect my arrays of arrays work.
>
>
> >
> > > +         ir_rvalue *lhs = new(ctx) ir_dereference_array(var,
> > > +                                             new(ctx) ir_constant(i));
> > > +         assignment = new(ctx) ir_assignment(lhs, rhs, NULL);
> > > +      } else {
> > > +         /* use writemask rather than index for vector */
> > > +         assert(var->type->is_vector());
> > > +         assert(i < 4);
> > > +         ir_dereference *lhs = new(ctx) ir_dereference_variable(var);
> > > +         assignment = new(ctx) ir_assignment(lhs, rhs, NULL,
> (unsigned)(1 << i));
> > > +      }
> > >
> > > -      ir_instruction *assignment = new(ctx) ir_assignment(lhs, rhs,
> NULL);
> > >        instructions->push_tail(assignment);
> > >
> > >        i++;
> >
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>


-- 
 Cody Northrop
 Graphics Software Engineer
 LunarG, Inc.- 3D Driver Innovations
 Email: cody at lunarg.com
 Website: http://www.lunarg.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140710/e145470a/attachment-0001.html>