On Mon, Dec 22, 2014 at 7:29 PM, Ben Widawsky
<benjamin.widaw...@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 <b...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
