[Mesa-dev] [PATCH 3/6] glsl: Generalize compute_packing_order for varying structs.

[Ian Romanick]

On 01/23/2013 11:16 AM, Paul Berry wrote:
> On 23 January 2013 07:38, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
>     On 01/21/2013 05:16 PM, Paul Berry wrote:
>
>         This patch paves the way for allowing varying structs by
>         generalizing
>         varying_matches::compute___packing_order to handle any type of
>         varying.
>         Previously, we packed in the order (vec4, vec2, float, vec3), with
>         matrices being packed according to the size of their columns.
>           Now, we
>         pack everything according to its number of components mod 4, in the
>         order (0, 2, 1, 3).
>
>         There is no behavioural change for vectors.  Matrices are now packed
>         slightly more sensibly (e.g. a mat2 is now packed with vec4's, which
>         guarantees that it will occupy a single varying slot, whereas it
>         previously was packed with vec2's, which meant that it might get
>         split
>         across two adjacent slots).
>         ---
>            src/glsl/link_varyings.cpp | 5 ++---
>            1 file changed, 2 insertions(+), 3 deletions(-)
>
>         diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
>         index 5a3240b..d8f501c 100644
>         --- a/src/glsl/link_varyings.cpp
>         +++ b/src/glsl/link_varyings.cpp
>         @@ -804,16 +804,15 @@
>         varying_matches::compute___packing_order(ir_variable *var)
>            {
>               const glsl_type *element_type = var->type;
>
>         -   /* FINISHME: Support for "varying" records in GLSL 1.50. */
>               while (element_type->base_type == GLSL_TYPE_ARRAY) {
>                  element_type = element_type->fields.array;
>               }
>
>         -   switch (element_type->vector___elements) {
>         +   switch (element_type->component___slots() % 4) {
>
>
>     Will this produce the same result for all matrix types as well?  I
>     think a mat2x3 would be 3 in the old scheme, but 2 in the new
>     scheme.  Right?
>
>
> Yes, a number of matrix types will get new packing orders.  Here's the
> complete list:
>
> - mat2x2 gets assigned PACKING_ORDER_VEC4 instead of
> PACKING_ORDER_VEC2.  As I argue above, this is slightly better, because
> it guarantees that the matrix occupies a single varying slot.
>
> - mat2x3 gets assigned PACKING_ORDER_VEC2 instead of
> PACKING_ORDER_VEC3.  This is kind of a wash.  Previously, mat2x3 had a
> 25% chance of having neither of its columns double parked, a 50% chance
> of having exactly one of its columns double parked, and a 25% chance of
> having both of its columns double parked.  Now it always has exactly one
> of its columns double parked.
>
> - mat3x3 gets assigned PACKING_ORDER_SCALAR instead of
> PACKING_ORDER_VEC3.  This doesn't affect much, since in both cases there
> is no guarantee of how the matrix will be aligned.
>
> - mat4x2 gets assigned PACKING_ORDER_VEC4 instead of
> PACKING_ORDER_VEC2.  I think this is slightly better for the same reason
> as in mat2x2.
>
> - mat4x3 gets assigned PACKING_ORDER_VEC4 instead of
> PACKING_ORDER_VEC3.  I think this is slightly better for the same reason
> as in mat2x2.

These changes potentially affect shader performance, do they also affect 
the number of slots used?  In other words, will a different set of 
shaders fit vs. not fit in 16 vec4 slots?

> Do you think I should include these details in the commit message?

Since you've already written it, the cut-and-paste couldn't hurt.

>               case 1: return PACKING_ORDER_SCALAR;
>               case 2: return PACKING_ORDER_VEC2;
>               case 3: return PACKING_ORDER_VEC3;
>         -   case 4: return PACKING_ORDER_VEC4;
>         +   case 0: return PACKING_ORDER_VEC4;
>               default:
>                  assert(!"Unexpected value of vector_elements");
>                  return PACKING_ORDER_VEC4;
>
>
>