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

[Ilia Mirkin]

FWIW you can't rely on things being packed. With something like SSO,
or enhanced layouts, you can get some funky stuff.
On Mon, Dec 10, 2018 at 9:22 AM Rob Clark <robdclark at gmail.com> wrote:
>
> 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) {
> > >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev