[Mesa-dev] [PATCH 3/3] nir: fixed some missed varying compaction opportunities

[Timothy Arceri]

I'd much rather land the first 3 patches from this series if possible.

https://patchwork.freedesktop.org/series/53800/

I've confirmed it packs the shaders you were looking at as expected once 
you patch 2 is applied. The series makes this code much more flexible 
(for future improvements) and easier to follow.

On 9/12/18 5:28 am, Rob Clark wrote:
> Previously, if we had a .z or .w component that could be compacted
> to .y, we'd could overlook that opportunity.
> 
> Signed-off-by: Rob Clark <robdclark at gmail.com>
> ---
>   src/compiler/nir/nir_linking_helpers.c | 30 ++++++++++++++++++++------
>   1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
> index 1ab9c095657..ce368a3c132 100644
> --- a/src/compiler/nir/nir_linking_helpers.c
> +++ b/src/compiler/nir/nir_linking_helpers.c
> @@ -431,12 +431,30 @@ compact_components(nir_shader *producer, nir_shader *consumer, uint8_t *comps,
>            uint8_t interp = get_interp_type(var, type, default_to_smooth_interp);
>            for (; cursor[interp] < 32; cursor[interp]++) {
>               uint8_t cursor_used_comps = comps[cursor[interp]];
> +            uint8_t unused_comps = ~cursor_used_comps;
>   
> -            /* We couldn't find anywhere to pack the varying continue on. */
> -            if (cursor[interp] == location &&
> -                (var->data.location_frac == 0 ||
> -                 cursor_used_comps & ((1 << (var->data.location_frac)) - 1)))
> -               break;
> +            /* Don't search beyond our current location, we are just trying
> +             * to pack later varyings to lower positions:
> +             */
> +            if (cursor[interp] == location) {
> +               if (var->data.location_frac == 0)
> +                  break;
> +
> +               /* If not already aligned to slot, see if we can shift it up.
> +                * Note that if we get this far it is a scalar so we know that
> +                * shifting this var to any other open position won't conflict
> +                * with it's current position.
> +                */
> +               unsigned p = ffs(unused_comps & 0xf);
> +               if (!p)
> +                  break;
> +
> +               /* ffs returns 1 for bit zero: */
> +               p--;
> +
> +               if (p >= var->data.location_frac)
> +                  break;
> +            }
>   
>               /* We can only pack varyings with matching interpolation types */
>               if (interp_type[cursor[interp]] != interp)
> @@ -460,8 +478,6 @@ compact_components(nir_shader *producer, nir_shader *consumer, uint8_t *comps,
>               if (!cursor_used_comps)
>                  continue;
>   
> -            uint8_t unused_comps = ~cursor_used_comps;
> -
>               for (unsigned i = 0; i < 4; i++) {
>                  uint8_t new_var_comps = 1 << i;
>                  if (unused_comps & new_var_comps) {
>