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

[Matt Turner]

On Mon, Feb 9, 2015 at 6:08 AM, Francisco Jerez <currojerez at 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)