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 > > +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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev