[Mesa-dev] [PATCH 10/21] i965/vec4: Address mul/mach constraints

[Matt Turner]

On Mon, Dec 22, 2014 at 7:29 PM, Ben Widawsky
<benjamin.widawsky at intel.com> wrote:
> This patch addresses the errata on GEN8+ which disallows the mul/mach macro to
> have a modifier on src1. This was previously listed as a FINISHME.

To be clear, I don't think this is an errata. It's just a natural
consequence of needing for force the source types to :W to read only
the low 16-bits. Using a source modifier would do incorrect things if,
for instance, the 15th bit was set but the 32-bit value wasn't
negative.

> Assertions for these cases will be added shortly.
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index 8d52a89..c8d1be6 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -1501,10 +1501,33 @@ vec4_visitor::visit(ir_expression *ir)
>     case ir_binop_imul_high: {
>        struct brw_reg acc = retype(brw_acc_reg(8), result_dst.type);
>        if (brw->gen >= 8) {
> +         /* GEN8+ 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.
> +          *
> +          * Unlike in the FS, we cannot just use a QWORD mul: "Restriction: Q/UQ
> +          * data types are not supported in Align16 mode."
> +          */
> +         src_reg temp;
> +         if (op[1].negate || op[1].abs) {
> +            if (!op[0].negate && !op[0].abs) {
> +               /* If op[0] has no modifiers, just swap */
> +               temp = op[0];
> +               op[0] = op[1];
> +               op[1] = temp;
> +            } else {
> +               /* Otherwise, apply via MOV first */
> +               src_reg temp = src_reg(this, glsl_type::uvec4_type);
> +               emit(BRW_OPCODE_MOV, dst_reg(temp), op[1]);
> +               op[1] = temp;
> +            }
> +         }

It's more difficult than this, isn't it? We need to ensure we don't
copy propagate things with source modifiers into the MUL/MACH as well.

>           emit(SHADER_OPCODE_MUL64, acc, op[0], op[1]);
>        } else {
>           emit(MUL(acc, op[0], op[1]));
>        }
> +

The whitespace change seems fine if it's intentional...

>        emit(MACH(result_dst, op[0], op[1]));
>        break;
>     }
> --
> 2.2.1