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

Submitted by Francisco Jerez on Feb. 9, 2015, 2:08 p.m.

Details

Message ID 1423490894-796-1-git-send-email-currojerez@riseup.net
State New
Headers show

Not browsing as part of any series.

Commit Message

Francisco Jerez Feb. 9, 2015, 2:08 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.

v2: Align multiple lines of an expression to the same column.  Change
    conditional compaction of 3-source instructions to an
    assertion. (Matt)
---
 src/mesa/drivers/dri/i965/brw_eu_compact.c | 46 ++++++++++++++++++++++++++++++
 1 file changed, 46 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..f80bcc1 100644
--- a/src/mesa/drivers/dri/i965/brw_eu_compact.c
+++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c
@@ -843,10 +843,53 @@  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)
 {
    assert(brw->gen >= 8);
+   assert(!has_3src_unmapped_bits(brw, src));
 
 #define compact(field) \
    brw_compact_inst_set_3src_##field(dst, brw_inst_3src_##field(brw, src))
@@ -937,6 +980,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 Mon, Feb 9, 2015 at 6:08 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.
>
> v2: Align multiple lines of an expression to the same column.  Change
>     conditional compaction of 3-source instructions to an
>     assertion. (Matt)
> ---
>  src/mesa/drivers/dri/i965/brw_eu_compact.c | 46 ++++++++++++++++++++++++++++++
>  1 file changed, 46 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..f80bcc1 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_compact.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c
> @@ -843,10 +843,53 @@ 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);

11 (NibCtrl) is the only one that isn't reserved. I'd rather assert
that reserved bits are not set.

> +   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));

Same thing, 47 (NibCtrl) is the only non-reserved bit. I'd rather
assert about the others.

> +}
> +
> +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) ||

105 is Src1's SubRegNum[word] on CHV and is included in SourceIndex.

> +             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);

Since bit 7 is reserved in all cases, we might just
assert(brw_inst_bits(src, 7,  7) == 0) in
brw_try_compact_instruction(). I don't have a preference.

So, I think it'd be nice to differentiate between bits that aren't
included in compact instructions and reserved bits.

(Sorry, I could have given you this feedback in the first time around)
Matt Turner <mattst88@gmail.com> writes:

> On Mon, Feb 9, 2015 at 6:08 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.
>>
>> v2: Align multiple lines of an expression to the same column.  Change
>>     conditional compaction of 3-source instructions to an
>>     assertion. (Matt)
>> ---
>>  src/mesa/drivers/dri/i965/brw_eu_compact.c | 46 ++++++++++++++++++++++++++++++
>>  1 file changed, 46 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..f80bcc1 100644
>> --- a/src/mesa/drivers/dri/i965/brw_eu_compact.c
>> +++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c
>> @@ -843,10 +843,53 @@ 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);
>
> 11 (NibCtrl) is the only one that isn't reserved. I'd rather assert
> that reserved bits are not set.
>
No, bits 47 and 95 are valid too and part of the AddrImm fields.

>> +   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));
>
> Same thing, 47 (NibCtrl) is the only non-reserved bit. I'd rather
> assert about the others.
>
>> +}
>> +
>> +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) ||
>
> 105 is Src1's SubRegNum[word] on CHV and is included in SourceIndex.
>
Ahh, that's true, good catch.

>> +             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);
>
> Since bit 7 is reserved in all cases, we might just
> assert(brw_inst_bits(src, 7,  7) == 0) in
> brw_try_compact_instruction(). I don't have a preference.
>
> So, I think it'd be nice to differentiate between bits that aren't
> included in compact instructions and reserved bits.
>
> (Sorry, I could have given you this feedback in the first time around)