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

[Michael Schellenberger]

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);
> +
> +   } else if (devinfo->gen >= 6) {
Could you make that ==6, so the two conditions do not overlap?
--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);