[Mesa-dev,32/32] i965: Don't compact instructions with unmapped bits.

Submitted by Francisco Jerez on Feb. 6, 2015, 2:43 p.m.

Details

Message ID 1423233792-11767-32-git-send-email-currojerez@riseup.net
State New
Headers show

Not browsing as part of any series.

Commit Message

Francisco Jerez Feb. 6, 2015, 2:43 p.m.
Some instruction bits don't have a mapping defined to any compacted
instruction field.  If they're ever set and we end up compacting the
instruction they will be forced to zero.  Avoid using compaction in such
cases.
---
 src/mesa/drivers/dri/i965/brw_eu_compact.c | 48 ++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_eu_compact.c b/src/mesa/drivers/dri/i965/brw_eu_compact.c
index 8e33bcb..7331ac8 100644
--- a/src/mesa/drivers/dri/i965/brw_eu_compact.c
+++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c
@@ -843,6 +843,48 @@  set_3src_source_index(struct brw_context *brw, brw_compact_inst *dst, brw_inst *
 }
 
 static bool
+has_unmapped_bits(struct brw_context *brw, brw_inst *src)
+{
+   /* Check for instruction bits that don't map to any of the fields of the
+    * compacted instruction.  The instruction cannot be compacted if any of
+    * them are set.  They overlap with:
+    *  - NibCtrl (bit 47 on Gen7, bit 11 on Gen8)
+    *  - Dst.AddrImm[9] (bit 47 on Gen8)
+    *  - Src0.AddrImm[9] (bit 95 on Gen8)
+    */
+   if (brw->gen >= 8)
+      return brw_inst_bits(src, 95, 95) ||
+         brw_inst_bits(src, 47, 47) ||
+         brw_inst_bits(src, 11, 11) ||
+         brw_inst_bits(src, 7,  7);
+   else
+      return brw_inst_bits(src, 95, 91) ||
+         brw_inst_bits(src, 47, 47) ||
+         brw_inst_bits(src, 7,  7) ||
+         (brw->gen < 7 && brw_inst_bits(src, 90, 90));
+}
+
+static bool
+has_3src_unmapped_bits(struct brw_context *brw, brw_inst *src)
+{
+   /* Check for three-source instruction bits that don't map to any of the
+    * fields of the compacted instruction.  All of them seem to be reserved
+    * bits currently.
+    */
+   assert(brw->gen >= 8);
+   if (brw->gen >= 9 || brw->is_cherryview)
+      return brw_inst_bits(src, 127, 127) ||
+         brw_inst_bits(src, 105, 105) ||
+         brw_inst_bits(src, 7,  7);
+   else
+      return brw_inst_bits(src, 127, 126) ||
+         brw_inst_bits(src, 105, 105) ||
+         brw_inst_bits(src, 84, 84) ||
+         brw_inst_bits(src, 36, 35) ||
+         brw_inst_bits(src, 7,  7);
+}
+
+static bool
 brw_try_compact_3src_instruction(struct brw_context *brw, brw_compact_inst *dst,
                                  brw_inst *src)
 {
@@ -851,6 +893,9 @@  brw_try_compact_3src_instruction(struct brw_context *brw, brw_compact_inst *dst,
 #define compact(field) \
    brw_compact_inst_set_3src_##field(dst, brw_inst_3src_##field(brw, src))
 
+   if (has_3src_unmapped_bits(brw, src))
+      return false;
+
    compact(opcode);
 
    if (!set_3src_control_index(brw, dst, src))
@@ -937,6 +982,9 @@  brw_try_compact_instruction(struct brw_context *brw, brw_compact_inst *dst,
       return false;
    }
 
+   if (has_unmapped_bits(brw, src))
+      return false;
+
    memset(&temp, 0, sizeof(temp));
 
    brw_compact_inst_set_opcode(&temp, brw_inst_opcode(brw, src));

Comments

On Fri, Feb 6, 2015 at 6:43 AM, Francisco Jerez <currojerez@riseup.net> wrote:
> Some instruction bits don't have a mapping defined to any compacted
> instruction field.  If they're ever set and we end up compacting the
> instruction they will be forced to zero.  Avoid using compaction in such
> cases.

Nice find!

> ---
>  src/mesa/drivers/dri/i965/brw_eu_compact.c | 48 ++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_compact.c b/src/mesa/drivers/dri/i965/brw_eu_compact.c
> index 8e33bcb..7331ac8 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_compact.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c
> @@ -843,6 +843,48 @@ set_3src_source_index(struct brw_context *brw, brw_compact_inst *dst, brw_inst *
>  }
>
>  static bool
> +has_unmapped_bits(struct brw_context *brw, brw_inst *src)
> +{
> +   /* Check for instruction bits that don't map to any of the fields of the
> +    * compacted instruction.  The instruction cannot be compacted if any of
> +    * them are set.  They overlap with:
> +    *  - NibCtrl (bit 47 on Gen7, bit 11 on Gen8)
> +    *  - Dst.AddrImm[9] (bit 47 on Gen8)
> +    *  - Src0.AddrImm[9] (bit 95 on Gen8)
> +    */
> +   if (brw->gen >= 8)
> +      return brw_inst_bits(src, 95, 95) ||
> +         brw_inst_bits(src, 47, 47) ||

Here and elsewhere, align the brw_inst_bits calls to the same column
as the call after the return.

> +         brw_inst_bits(src, 11, 11) ||
> +         brw_inst_bits(src, 7,  7);
> +   else
> +      return brw_inst_bits(src, 95, 91) ||
> +         brw_inst_bits(src, 47, 47) ||
> +         brw_inst_bits(src, 7,  7) ||
> +         (brw->gen < 7 && brw_inst_bits(src, 90, 90));
> +}
> +
> +static bool
> +has_3src_unmapped_bits(struct brw_context *brw, brw_inst *src)
> +{
> +   /* Check for three-source instruction bits that don't map to any of the
> +    * fields of the compacted instruction.  All of them seem to be reserved
> +    * bits currently.

If they're all reserved bits we should never be setting them, so we
should just assert.