[Mesa-dev] [PATCH] i965/vec4: Don't coalesce registers in gen6 math ops if reswizzling needed

[Antía Puentes]

On dom, 2015-09-20 at 11:11 -0700, Matt Turner wrote:
> On Sun, Sep 20, 2015 at 8:48 AM, Antia Puentes <apuentes at igalia.com> wrote:
> > Math operations in SandyBridge do not support source swizzling
> 
> I can't find any documentation to support this claim, but I remember
> that SNB math must be in align1 mode so it can't do swizzles or
> writemasking (see commit e14cc504).

I realized that if writemasking is also unsupported I should probably
update the patch to avoid coalescing if the dst_writemask parameter is
different from XYWZ. The condition in "vec4_instruction::can_reswizzle"
would be something like:

  /* Gen6 MATH instructions can not execute in align16 mode, so swizzles
   * or writemasking are not allowed. */
   if (devinfo->gen == 6 && is_math() &&
       swizzle != BRW_SWIZZLE_XYZW &&
       dst_writemask != WRITEMASK_XYWZ)
      return false;

The writemask check is needed because the "vec4_instruction::reswizzle"
method, apart from modifying the swizzle, also updates the intruction's
writemask to the AND between the original writemask and the writemask of
the instruction we are trying to remove.

I will run Piglit and dEQP and send the updated patch.

> But, the documentation actually says "The supported regioning modes
> for math instruction are align16, align1 with the following
> restrictions: ..."
> 
> Would be nice if we could actually find a PRM citation.
> 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92033
> > ---
> >  src/mesa/drivers/dri/i965/brw_ir_vec4.h |  3 ++-
> >  src/mesa/drivers/dri/i965/brw_vec4.cpp  | 11 +++++++++--
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_ir_vec4.h b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> > index 966a410..a48bb68 100644
> > --- a/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> > +++ b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> > @@ -175,7 +175,8 @@ public:
> >
> >     bool is_send_from_grf();
> >     unsigned regs_read(unsigned arg) const;
> > -   bool can_reswizzle(int dst_writemask, int swizzle, int swizzle_mask);
> > +   bool can_reswizzle(const struct brw_device_info *devinfo, int dst_writemask,
> > +                      int swizzle, int swizzle_mask);
> >     void reswizzle(int dst_writemask, int swizzle);
> >     bool can_do_source_mods(const struct brw_device_info *devinfo);
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> > index ed49cd3..d7192e4 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> > @@ -941,10 +941,17 @@ vec4_visitor::opt_set_dependency_control()
> >  }
> >
> >  bool
> > -vec4_instruction::can_reswizzle(int dst_writemask,
> > +vec4_instruction::can_reswizzle(const struct brw_device_info *devinfo,
> > +                                int dst_writemask,
> >                                  int swizzle,
> >                                  int swizzle_mask)
> >  {
> > +
> 
> Extra new line.
> 
> > +   /* gen6 math instructions can not manage source swizzles */
> 
> If we can find documentation, we should update this comment. If not,
> let's change it to read
> 
> /* Gen6 MATH instructions can not execute in align16 mode, so swizzles
> are not allowed. */
> 
> (linewrapped as appropriate)
> 
> With those changes,
> 
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>