[Mesa-dev] [PATCH 08/10] glsl: Add a lowering pass for packing varyings.

[Paul Berry]

On 12 December 2012 14:49, Eric Anholt <eric at anholt.net> wrote:

> Paul Berry <stereotype441 at gmail.com> writes:
> > diff --git a/src/glsl/lower_packed_varyings.cpp
> b/src/glsl/lower_packed_varyings.cpp
> > new file mode 100644
> > index 0000000..4cb9066
> > --- /dev/null
> > +++ b/src/glsl/lower_packed_varyings.cpp
> > @@ -0,0 +1,371 @@
> > +/*
> > + * Copyright © 2011 Intel Corporation
>
>                   2012
>
> > +/**
> > + * \file lower_varyings_to_packed.cpp
>
>             lower_packed_varyings.cpp
>
> > +   /**
> > +    * Number of generic varying slots which are used by this shader.
>  This is
> > +    * used to allocate temporary intermediate data structures.  If any
> any
>
> Too many "any"s.
>
> > +    * varying used by this shader has a location greater than or equal
> to
> > +    * location_base + locations_used, an assertion will fire.
> > +    */
> > +   const unsigned locations_used;
>
>
> > +ir_visitor_status
> > +lower_packed_varyings_visitor::visit(ir_variable *var)
> > +{
> > +   if (var->mode != this->mode ||
> > +       var->location < (int) this->location_base ||
> > +       !this->needs_lowering(var))
> > +      return visit_continue;
> > +
> > +   /* Change the old varying into an ordinary global. */
> > +   var->mode = ir_var_auto;
> > +
> > +   /* Create a reference to the old varying. */
> > +   ir_dereference_variable *deref
> > +      = new(this->mem_ctx) ir_dereference_variable(var);
> > +
> > +   /* Recursively pack or unpack it. */
> > +   this->lower_rvalue(deref, var->location * 4 + var->location_frac,
> var);
> > +
> > +   return visit_continue;
> > +}
>
> I think an alternative here, instead of using the visitor class and
> skipping functions, would be to just walk the top-level ir_instruction
> list, check if it's a variable, and the run this function body on it.
>

Yeah, I considered that too.  I don't really have a strong preference
either way.


>
> > +unsigned
> > +lower_packed_varyings_visitor::lower_rvalue(ir_rvalue *rvalue,
> > +                                            unsigned fine_location,
> > +                                            ir_variable *unpacked_var)
> > +{
>
> I will admit I didn't totally follow the path of the swizzles through
> this code, but that's my fault and I expect testing to have covered it.
>
> > +   unsigned slot = location - this->location_base;
> > +   assert(slot < locations_used);
> > +   if (this->packed_varyings[slot] == NULL) {
> > +      char name[10];
> > +      sprintf(name, "packed%d", slot);
>
> Bonus points if this could also strcat the variable names contributing
> to it somehow.  Turning all my FS inputs into packedN is ugly for shader
> debugging.
>

Yeah, I can see why that would be useful.  I'll spend a little time on it
and see if I can do it without complicating the code too much--if I can't,
I'll leave the bonus points available for someone else to pick up :)


>
> > +      const glsl_type *packed_type;
> > +      switch (unpacked_var->type->get_scalar_type()->base_type) {
> > +      case GLSL_TYPE_UINT:
> > +         packed_type = glsl_type::uvec4_type;
> > +         break;
> > +      case GLSL_TYPE_INT:
> > +         packed_type = glsl_type::ivec4_type;
> > +         break;
> > +      case GLSL_TYPE_FLOAT:
> > +         packed_type = glsl_type::vec4_type;
> > +         break;
> > +      case GLSL_TYPE_BOOL:
> > +         /* Note: in theory bools aren't supposed to be allowed to be
> > +          * varyings, however Mesa doesn't know to reject them yet.
>  Until
> > +          * this is fixed, we might as well allow them to be packed too.
> > +          */
> > +         packed_type = glsl_type::bvec4_type;
> > +         break;
> > +      default:
> > +         assert(!"Unexpected varying type while packing");
> > +         packed_type = glsl_type::vec4_type;
> > +         break;
> > +      }
>
> Maybe just:
>
> packed_type = get_instance(unpacked_var->get_scalar_type()->base_type, 4,
> 1);
>

Aha, yes!  That's way better.


>
> At this point I've read through the series, and only had trivial
> review/suggestions.  Fix what you like and:
>
> Reviewed-by: Eric Anholt <eric at anholt.net>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20121212/c02b9d4e/attachment-0001.html>