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

[Ben Widawsky]

On Tue, Dec 23, 2014 at 10:45:27AM -0800, Matt Turner wrote:
> 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.
> 

You are right, I shouldn't have said it's errata, and it's not GEN8 specific
either.

> > 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.
> 

Perhaps. I don't know much about copy propagation, but I will investigate. 

A similar issue came up in doing the multiplication stuff. It seems like at
basically every optimization stage there is a chance of screwing things up that
were changed around in the visitor. The simple response seems to be, don't do
these things in the visitor, however in these cases it is often more convenient
to do so.

Do we have some easy way of expressing what the optimization passes can and
cannot modify? I'm this doesn't imply the patches basically need to be
rewritten.

> >           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