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

[Eric Anholt]

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.

> +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.

> +      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);

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 --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20121212/288db2b9/attachment.pgp>