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

[Rob Clark]

That is fine by me.  I've come up with a workaround for now (just
adding some dummy output components to handle the .x__w case) so at
least he .w varying doesn't get lost with that shader.

BR,
-R

On Sun, Dec 9, 2018 at 8:20 PM Timothy Arceri <tarceri at itsqueeze.com> wrote:
>
> 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) {
> >