[Mesa-dev] [PATCH 01/21] i965/eu: Define alternative interface for setting compression and group controls.

[Jason Ekstrand]

On Tue, May 24, 2016 at 2:05 AM, Michael Schellenberger <
mschellenbergercosta at googlemail.com> wrote:

> Hi curro,
>
> Am 24.05.2016 um 09:18 schrieb Francisco Jerez:
> > This implements some simple helper functions that can be used to
> > specify the group of channel enable signals and compression enable
> > that apply to a brw_inst instruction.
> >
> > It's intended to replace brw_set_default_compression_control
> > eventually because the current interface has a number of shortcomings
> > inherited from the Gen-4-5-centric representation of compression and
> > group controls as a single non-orthogonal enum: On the one hand it
> > doesn't work for specifying arbitrary group controls other than 1Q and
> > 2Q, which are frequently useful in SIMD32 and FP64 programs.  On the
> > other hand the current interface forces you to update the compression
> > *and* group controls simultaneously, which has been the source of a
> > number of generator bugs (a bunch of them fixed in this series),
> > because in many cases we would end up resetting the group controls to
> > zero inadvertently even though everything we wanted to do was disable
> > instruction compression -- The latter seems especially unfortunate on
> > Gen6+ hardware which have no explicit compression control, so we would
> > end up bashing the quarter control field of the instruction for no
> > benefit.
> >
> > Instead of a single function that updates both at the same time
> > introduce separate interfaces to update one or the other independently
> > preserving the current value of the other (which typically comes from
> > the back-end IR so it has to be respected).
> > ---
> >  src/mesa/drivers/dri/i965/brw_eu.c | 69
> ++++++++++++++++++++++++++++++++++++++
> >  src/mesa/drivers/dri/i965/brw_eu.h |  6 ++++
> >  2 files changed, 75 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_eu.c
> b/src/mesa/drivers/dri/i965/brw_eu.c
> > index 48c8439..f1161d2 100644
> > --- a/src/mesa/drivers/dri/i965/brw_eu.c
> > +++ b/src/mesa/drivers/dri/i965/brw_eu.c
> > @@ -218,6 +218,75 @@ brw_set_default_compression_control(struct
> brw_codegen *p,
> >     }
> >  }
> >
> > +/**
> > + * Enable or disable instruction compression on the given instruction
> leaving
> > + * the currently selected channel enable group untouched.
> > + */
> > +void
> > +brw_inst_set_compression(const struct brw_device_info *devinfo,
> > +                         brw_inst *inst, bool on)
> > +{
> > +   if (devinfo->gen >= 6) {
> > +      /* No-op, the EU will figure out for us whether the instruction
> needs to
> > +       * be compressed.
> > +       */
> > +   } else {
> > +      /* The channel group and compression controls are non-orthogonal,
> there
> > +       * are two possible representations for uncompressed instructions
> and we
> > +       * may need to preserve the current one to avoid changing the
> selected
> > +       * channel group inadvertently.
> > +       */
> > +      if (on)
> > +         brw_inst_set_qtr_control(devinfo, inst,
> BRW_COMPRESSION_COMPRESSED);
> > +      else if (brw_inst_qtr_control(devinfo, inst)
> > +               == BRW_COMPRESSION_COMPRESSED)
> > +         brw_inst_set_qtr_control(devinfo, inst, BRW_COMPRESSION_NONE);
> > +   }
> > +}
> > +
> > +void
> > +brw_set_default_compression(struct brw_codegen *p, bool on)
> > +{
> > +   brw_inst_set_compression(p->devinfo, p->current, on);
> > +}
> > +
> > +/**
> > + * Apply the range of channel enable signals given by
> > + * [group, group + exec_size[ to the instruction passed as argument.
> > + */
> > +void
> > +brw_inst_set_group(const struct brw_device_info *devinfo,
> > +                   brw_inst *inst, unsigned group)
> > +{
> > +   if (devinfo->gen >= 7) {
> > +      assert(group % 4 == 0 && group < 32);
> > +      brw_inst_set_qtr_control(devinfo, inst, group / 8);
> > +      brw_inst_set_nib_control(devinfo, inst, group / 4 % 2);
>

Mind adding parentheses around "group / 4" so the reader doesn't have to
think about operator precedence between / and %.


> > +
> > +   } else if (devinfo->gen >= 6) {
> Could you make that ==6, so the two conditions do not overlap?
>

That's not a bad idea.


> --Michael
> > +      assert(group % 8 == 0 && group < 32);
> > +      brw_inst_set_qtr_control(devinfo, inst, group / 8);
> > +
> > +   } else {
> > +      assert(group % 8 == 0 && group < 16);
> > +      /* The channel group and compression controls are non-orthogonal,
> there
> > +       * are two possible representations for group zero and we may
> need to
> > +       * preserve the current one to avoid changing the selected
> compression
> > +       * enable inadvertently.
> > +       */
> > +      if (group == 8)
> > +         brw_inst_set_qtr_control(devinfo, inst,
> BRW_COMPRESSION_2NDHALF);
> > +      else if (brw_inst_qtr_control(devinfo, inst) ==
> BRW_COMPRESSION_2NDHALF)
> > +         brw_inst_set_qtr_control(devinfo, inst, BRW_COMPRESSION_NONE);
> > +   }
> > +}
> > +
> > +void
> > +brw_set_default_group(struct brw_codegen *p, unsigned group)
> > +{
> > +   brw_inst_set_group(p->devinfo, p->current, group);
> > +}
> > +
> >  void brw_set_default_mask_control( struct brw_codegen *p, unsigned
> value )
> >  {
> >     brw_inst_set_mask_control(p->devinfo, p->current, value);
> > diff --git a/src/mesa/drivers/dri/i965/brw_eu.h
> b/src/mesa/drivers/dri/i965/brw_eu.h
> > index bea90f4..03400ae 100644
> > --- a/src/mesa/drivers/dri/i965/brw_eu.h
> > +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> > @@ -101,6 +101,12 @@ void brw_set_default_exec_size(struct brw_codegen
> *p, unsigned value);
> >  void brw_set_default_mask_control( struct brw_codegen *p, unsigned
> value );
> >  void brw_set_default_saturate( struct brw_codegen *p, bool enable );
> >  void brw_set_default_access_mode( struct brw_codegen *p, unsigned
> access_mode );
> > +void brw_inst_set_compression(const struct brw_device_info *devinfo,
> > +                              brw_inst *inst, bool on);
> > +void brw_set_default_compression(struct brw_codegen *p, bool on);
> > +void brw_inst_set_group(const struct brw_device_info *devinfo,
> > +                        brw_inst *inst, unsigned group);
> > +void brw_set_default_group(struct brw_codegen *p, unsigned group);
> >  void brw_set_default_compression_control(struct brw_codegen *p, enum
> brw_compression c);
> >  void brw_set_default_predicate_control( struct brw_codegen *p, unsigned
> pc );
> >  void brw_set_default_predicate_inverse(struct brw_codegen *p, bool
> predicate_inverse);
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160524/f763381d/attachment.html>