On 12 December 2012 14:49, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>
> diff --git a/src/glsl/lower_packed_varyings.cpp b/src/glsl/lower_packed_varyings.cpp<br>
> new file mode 100644<br>
> index 0000000..4cb9066<br>
> --- /dev/null<br>
> +++ b/src/glsl/lower_packed_varyings.cpp<br>
> @@ -0,0 +1,371 @@<br>
> +/*<br>
> + * Copyright © 2011 Intel Corporation<br>
<br>
</div>                  2012<br>
<div class="im"><br>
> +/**<br>
> + * \file lower_varyings_to_packed.cpp<br>
<br>
</div>            lower_packed_varyings.cpp<br>
<div class="im"><br>
> +   /**<br>
> +    * Number of generic varying slots which are used by this shader.  This is<br>
> +    * used to allocate temporary intermediate data structures.  If any any<br>
<br>
</div>Too many "any"s.<br>
<div class="im"><br>
> +    * varying used by this shader has a location greater than or equal to<br>
> +    * location_base + locations_used, an assertion will fire.<br>
> +    */<br>
> +   const unsigned locations_used;<br>
<br>
<br>
</div><div class="im">> +ir_visitor_status<br>
> +lower_packed_varyings_visitor::visit(ir_variable *var)<br>
> +{<br>
> +   if (var->mode != this->mode ||<br>
> +       var->location < (int) this->location_base ||<br>
> +       !this->needs_lowering(var))<br>
> +      return visit_continue;<br>
> +<br>
> +   /* Change the old varying into an ordinary global. */<br>
> +   var->mode = ir_var_auto;<br>
> +<br>
> +   /* Create a reference to the old varying. */<br>
> +   ir_dereference_variable *deref<br>
> +      = new(this->mem_ctx) ir_dereference_variable(var);<br>
> +<br>
> +   /* Recursively pack or unpack it. */<br>
> +   this->lower_rvalue(deref, var->location * 4 + var->location_frac, var);<br>
> +<br>
> +   return visit_continue;<br>
> +}<br>
<br>
</div>I think an alternative here, instead of using the visitor class and<br>
skipping functions, would be to just walk the top-level ir_instruction<br>
list, check if it's a variable, and the run this function body on it.<br></blockquote><div><br>Yeah, I considered that too.  I don't really have a strong preference either way.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="im"><br>
> +unsigned<br>
> +lower_packed_varyings_visitor::lower_rvalue(ir_rvalue *rvalue,<br>
> +                                            unsigned fine_location,<br>
> +                                            ir_variable *unpacked_var)<br>
> +{<br>
<br>
</div>I will admit I didn't totally follow the path of the swizzles through<br>
this code, but that's my fault and I expect testing to have covered it.<br>
<div class="im"><br>
> +   unsigned slot = location - this->location_base;<br>
> +   assert(slot < locations_used);<br>
> +   if (this->packed_varyings[slot] == NULL) {<br>
> +      char name[10];<br>
> +      sprintf(name, "packed%d", slot);<br>
<br>
</div>Bonus points if this could also strcat the variable names contributing<br>
to it somehow.  Turning all my FS inputs into packedN is ugly for shader<br>
debugging.<br></blockquote><div><br>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 :)<br>
 </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> +      const glsl_type *packed_type;<br>
> +      switch (unpacked_var->type->get_scalar_type()->base_type) {<br>
> +      case GLSL_TYPE_UINT:<br>
> +         packed_type = glsl_type::uvec4_type;<br>
> +         break;<br>
> +      case GLSL_TYPE_INT:<br>
> +         packed_type = glsl_type::ivec4_type;<br>
> +         break;<br>
> +      case GLSL_TYPE_FLOAT:<br>
> +         packed_type = glsl_type::vec4_type;<br>
> +         break;<br>
> +      case GLSL_TYPE_BOOL:<br>
> +         /* Note: in theory bools aren't supposed to be allowed to be<br>
> +          * varyings, however Mesa doesn't know to reject them yet.  Until<br>
> +          * this is fixed, we might as well allow them to be packed too.<br>
> +          */<br>
> +         packed_type = glsl_type::bvec4_type;<br>
> +         break;<br>
> +      default:<br>
> +         assert(!"Unexpected varying type while packing");<br>
> +         packed_type = glsl_type::vec4_type;<br>
> +         break;<br>
> +      }<br>
<br>
</div>Maybe just:<br>
<br>
packed_type = get_instance(unpacked_var->get_scalar_type()->base_type, 4, 1);<br></blockquote><div><br>Aha, yes!  That's way better.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
At this point I've read through the series, and only had trivial<br>
review/suggestions.  Fix what you like and:<br>
<br>
Reviewed-by: Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>><br>
</blockquote></div><br></div>