Re: [Mesa-dev] [PATCH 01/21] i965/eu: Define alternative interface for setting compression and group controls.
On Tue, May 24, 2016 at 2:05 AM, Michael Schellenberger <
mschellenbergerco...@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
> >
Re: [Mesa-dev] [PATCH 01/21] i965/eu: Define alternative interface for setting compression and group controls.
On Tue, May 24, 2016 at 2:05 AM, Michael Schellenberger <
mschellenbergerco...@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
> >
Re: [Mesa-dev] [PATCH 01/21] i965/eu: Define alternative interface for setting compression and group controls.
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/
