<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 24, 2016 at 2:05 AM, Michael Schellenberger <span dir="ltr"><<a href="mailto:mschellenbergercosta@googlemail.com" target="_blank">mschellenbergercosta@googlemail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi curro,<br>
<div><div class="h5"><br>
Am 24.05.2016 um 09:18 schrieb Francisco Jerez:<br>
> This implements some simple helper functions that can be used to<br>
> specify the group of channel enable signals and compression enable<br>
> that apply to a brw_inst instruction.<br>
><br>
> It's intended to replace brw_set_default_compression_control<br>
> eventually because the current interface has a number of shortcomings<br>
> inherited from the Gen-4-5-centric representation of compression and<br>
> group controls as a single non-orthogonal enum: On the one hand it<br>
> doesn't work for specifying arbitrary group controls other than 1Q and<br>
> 2Q, which are frequently useful in SIMD32 and FP64 programs.  On the<br>
> other hand the current interface forces you to update the compression<br>
> *and* group controls simultaneously, which has been the source of a<br>
> number of generator bugs (a bunch of them fixed in this series),<br>
> because in many cases we would end up resetting the group controls to<br>
> zero inadvertently even though everything we wanted to do was disable<br>
> instruction compression -- The latter seems especially unfortunate on<br>
> Gen6+ hardware which have no explicit compression control, so we would<br>
> end up bashing the quarter control field of the instruction for no<br>
> benefit.<br>
><br>
> Instead of a single function that updates both at the same time<br>
> introduce separate interfaces to update one or the other independently<br>
> preserving the current value of the other (which typically comes from<br>
> the back-end IR so it has to be respected).<br>
> ---<br>
>  src/mesa/drivers/dri/i965/brw_eu.c | 69 ++++++++++++++++++++++++++++++++++++++<br>
>  src/mesa/drivers/dri/i965/brw_eu.h |  6 ++++<br>
>  2 files changed, 75 insertions(+)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.c b/src/mesa/drivers/dri/i965/brw_eu.c<br>
> index 48c8439..f1161d2 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_eu.c<br>
> +++ b/src/mesa/drivers/dri/i965/brw_eu.c<br>
> @@ -218,6 +218,75 @@ brw_set_default_compression_control(struct brw_codegen *p,<br>
>     }<br>
>  }<br>
><br>
> +/**<br>
> + * Enable or disable instruction compression on the given instruction leaving<br>
> + * the currently selected channel enable group untouched.<br>
> + */<br>
> +void<br>
> +brw_inst_set_compression(const struct brw_device_info *devinfo,<br>
> +                         brw_inst *inst, bool on)<br>
> +{<br>
> +   if (devinfo->gen >= 6) {<br>
> +      /* No-op, the EU will figure out for us whether the instruction needs to<br>
> +       * be compressed.<br>
> +       */<br>
> +   } else {<br>
> +      /* The channel group and compression controls are non-orthogonal, there<br>
> +       * are two possible representations for uncompressed instructions and we<br>
> +       * may need to preserve the current one to avoid changing the selected<br>
> +       * channel group inadvertently.<br>
> +       */<br>
> +      if (on)<br>
> +         brw_inst_set_qtr_control(devinfo, inst, BRW_COMPRESSION_COMPRESSED);<br>
> +      else if (brw_inst_qtr_control(devinfo, inst)<br>
> +               == BRW_COMPRESSION_COMPRESSED)<br>
> +         brw_inst_set_qtr_control(devinfo, inst, BRW_COMPRESSION_NONE);<br>
> +   }<br>
> +}<br>
> +<br>
> +void<br>
> +brw_set_default_compression(struct brw_codegen *p, bool on)<br>
> +{<br>
> +   brw_inst_set_compression(p->devinfo, p->current, on);<br>
> +}<br>
> +<br>
> +/**<br>
> + * Apply the range of channel enable signals given by<br>
> + * [group, group + exec_size[ to the instruction passed as argument.<br>
> + */<br>
> +void<br>
> +brw_inst_set_group(const struct brw_device_info *devinfo,<br>
> +                   brw_inst *inst, unsigned group)<br>
> +{<br>
> +   if (devinfo->gen >= 7) {<br>
> +      assert(group % 4 == 0 && group < 32);<br>
> +      brw_inst_set_qtr_control(devinfo, inst, group / 8);<br>
> +      brw_inst_set_nib_control(devinfo, inst, group / 4 % 2);<br></div></div></blockquote><div><br></div><div>Mind adding parentheses around "group / 4" so the reader doesn't have to think about operator precedence between / and %.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> +<br>
> +   } else if (devinfo->gen >= 6) {<br>
</div></div>Could you make that ==6, so the two conditions do not overlap?<br></blockquote><div><br></div><div>That's not a bad idea.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888">--Michael<br>
</font></span><div class="HOEnZb"><div class="h5">> +      assert(group % 8 == 0 && group < 32);<br>
> +      brw_inst_set_qtr_control(devinfo, inst, group / 8);<br>
> +<br>
> +   } else {<br>
> +      assert(group % 8 == 0 && group < 16);<br>
> +      /* The channel group and compression controls are non-orthogonal, there<br>
> +       * are two possible representations for group zero and we may need to<br>
> +       * preserve the current one to avoid changing the selected compression<br>
> +       * enable inadvertently.<br>
> +       */<br>
> +      if (group == 8)<br>
> +         brw_inst_set_qtr_control(devinfo, inst, BRW_COMPRESSION_2NDHALF);<br>
> +      else if (brw_inst_qtr_control(devinfo, inst) == BRW_COMPRESSION_2NDHALF)<br>
> +         brw_inst_set_qtr_control(devinfo, inst, BRW_COMPRESSION_NONE);<br>
> +   }<br>
> +}<br>
> +<br>
> +void<br>
> +brw_set_default_group(struct brw_codegen *p, unsigned group)<br>
> +{<br>
> +   brw_inst_set_group(p->devinfo, p->current, group);<br>
> +}<br>
> +<br>
>  void brw_set_default_mask_control( struct brw_codegen *p, unsigned value )<br>
>  {<br>
>     brw_inst_set_mask_control(p->devinfo, p->current, value);<br>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h b/src/mesa/drivers/dri/i965/brw_eu.h<br>
> index bea90f4..03400ae 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_eu.h<br>
> +++ b/src/mesa/drivers/dri/i965/brw_eu.h<br>
> @@ -101,6 +101,12 @@ void brw_set_default_exec_size(struct brw_codegen *p, unsigned value);<br>
>  void brw_set_default_mask_control( struct brw_codegen *p, unsigned value );<br>
>  void brw_set_default_saturate( struct brw_codegen *p, bool enable );<br>
>  void brw_set_default_access_mode( struct brw_codegen *p, unsigned access_mode );<br>
> +void brw_inst_set_compression(const struct brw_device_info *devinfo,<br>
> +                              brw_inst *inst, bool on);<br>
> +void brw_set_default_compression(struct brw_codegen *p, bool on);<br>
> +void brw_inst_set_group(const struct brw_device_info *devinfo,<br>
> +                        brw_inst *inst, unsigned group);<br>
> +void brw_set_default_group(struct brw_codegen *p, unsigned group);<br>
>  void brw_set_default_compression_control(struct brw_codegen *p, enum brw_compression c);<br>
>  void brw_set_default_predicate_control( struct brw_codegen *p, unsigned pc );<br>
>  void brw_set_default_predicate_inverse(struct brw_codegen *p, bool predicate_inverse);<br>
<br>
</div></div><div class="HOEnZb"><div class="h5">_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></div><br></div></div>