[Mesa-dev] [PATCH 11/21] i965: Add assertions for MACH instruction

[Matt Turner]

On Mon, Dec 22, 2014 at 7:29 PM, Ben Widawsky
<benjamin.widawsky at intel.com> wrote:
> "A source modifier must not be used on src1 for the macro operation. This
> applies to both mul and mach of the macro. If source modifier is required, an
> additional mov instruction may be used before the macro."
>
> Today, we only use MACH in the macro, so this assertion should always hold.
>
> This assertion applies to mul too as mentioned above. It is harder to detect mul
> when used in the macro operation, so that is left for now.

A MUL is used in a macro operation if and only if its destination if
the accumulator, right?

Assuming so, we should do the assertions in brw_MUL.

> "Accumulator is an implicit source and thus cannot be an explicit source
> operand."
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_eu_emit.c | 46 ++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index 046e293..51bf1b9 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -1032,7 +1032,6 @@ ALU2(ASR)
>  ALU1(FRC)
>  ALU1(RNDD)
>  ALU2(MAC)
> -ALU2(MACH)
>  ALU1(LZD)
>  ALU2(DP4)
>  ALU2(DPH)
> @@ -1136,6 +1135,51 @@ brw_LINE(struct brw_compile *p, struct brw_reg dest,
>  }
>
>  brw_inst *
> +brw_MACH(struct brw_compile *p, struct brw_reg dest,
> +         struct brw_reg src0, struct brw_reg src1)
> +{
> +   const struct brw_context *brw = p->brw;
> +   (void) brw; /* Get rid of unused variable warning with NDEBUG */

We do this enough that no comment is needed.

> +
> +   /* A source modifier must not be used on src1 for the macro operation. */
> +   assert(brw->gen < 8 || !src1.negate);
> +   assert(brw->gen < 8 || !src1.abs);
> +
> +   /* Source and destination operands must be DWord integers. Source and
> +    * destination must be of the same type, signed integer or unsigned integer.
> +    */
> +   assert(brw->gen < 8 || src0.type == src1.type);
> +   assert(brw->gen < 8 || type_is_dword(src0.type));
> +
> +   /* If dst is UD, src0 and src1 may be UD and/or D. However, if any of src0
> +    * and src1 is D, source modifier (abs) must be present to convert it to
> +    * match with dst.
> +    *
> +    * NOTE: There is a src1 exception above
> +    */
> +   assert(brw->gen < 8 ||
> +          dest.type == BRW_REGISTER_TYPE_D ||
> +          (src1.type == BRW_REGISTER_TYPE_UD &&
> +           src0.type == BRW_REGISTER_TYPE_UD) ||

Weird to test src1 and then src0, but okay.

> +          (src0.negate));

Extra parenthesis around src0.negate.

> +
> +   /* If dest is D, src0 and src1 must also be D. They cannot be UD as it may
> +    * cause unexpected overflow because the computed results are limited to 64
> +    * bits.
> +    */
> +   assert(brw->gen < 8 ||
> +          dest.type != BRW_REGISTER_TYPE_D ||
> +          (src1.type == BRW_REGISTER_TYPE_D && src0.type == BRW_REGISTER_TYPE_D));

src1 and then src0. Weird but okay.
> +
> +   assert(src0.file != BRW_ARCHITECTURE_REGISTER_FILE ||
> +         src0.nr != BRW_ARF_ACCUMULATOR);
> +   assert(src1.file != BRW_ARCHITECTURE_REGISTER_FILE ||
> +         src1.nr != BRW_ARF_ACCUMULATOR);

Some tabs in this patch. Spaces only.