[Mesa-dev] [PATCH 04/15] nir: add doubles component packing support

[Alejandro Piñeiro]

On 19/07/16 08:33, Timothy Arceri wrote:
> This makes sure we give the correct driver location
> for doubles when using component packing.

Taking into account that the commit is about give the correct location
for the dvec3 case, probably it is worth to mention on the commit message.

> ---
>  src/compiler/nir/nir_lower_io.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/src/compiler/nir/nir_lower_io.c b/src/compiler/nir/nir_lower_io.c
> index e480264..7a72e69 100644
> --- a/src/compiler/nir/nir_lower_io.c
> +++ b/src/compiler/nir/nir_lower_io.c
> @@ -75,6 +75,22 @@ nir_assign_var_locations(struct exec_list *var_list, unsigned *size,
>           if (locations[idx][var->data.index] == -1) {
>              var->data.driver_location = location;
>              locations[idx][var->data.index] = location;
> +
> +            /* A dvec3 can be packed with a double we need special handling
> +             * for this as we are packing across two locations.
> +             */
> +            if (glsl_get_base_type(var->type) == GLSL_TYPE_DOUBLE &&
> +                glsl_get_vector_elements(var->type) == 3) {
> +               /* Hack around type_size functions that expect vectors to be
> +                * padded out to vec4.
> +                */
> +               unsigned dsize = type_size(glsl_double_type());
> +               unsigned offset =
> +                  dsize == type_size(glsl_float_type()) ? dsize : dsize * 2;

This is hackish indeed, and somewhat complex to understand (assuming
that Im properly understanding it).  Assuming that there isn't a better
way to express this on code, and in the risk of becoming an
add-more-comments-pain-in-the-neck, probably it is worth to expand a
little the comment below.

> +
> +               locations[idx + 1][var->data.index] = location + offset;
> +            }
> +
>              location += type_size(var->type);
>           } else {
>              var->data.driver_location = locations[idx][var->data.index];

In any case:
Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>