[Mesa-dev] [PATCH] i965/gs: Avoid DW * DW mul

[Ben Widawsky]

On Thu, Dec 04, 2014 at 05:17:05PM -0800, Kenneth Graunke wrote:
> On Thursday, December 04, 2014 03:37:17 PM Ben Widawsky wrote:
> > The GS has an interesting use for mul. It's essentially used as a fancy mov (in
> > fact, I am not sure why a mov isn't used). The documentation in the function has
> > a very good explanation from Paul on the mechanics.
> > 
> > CHV has some quirks with regard to multiplication. While the documentation is
> > somewhat unclear, I've found that demoting the src1 operand in the GS mul solves
> > all the problems. I'd ask that any potential reviewer ignore the other instances
> > of mul for now (I have more patches), and simply make sure that what this patch
> > does is correct.
> 
> (probably don't need review suggestions in the commit message; these normally
>  go below the --- line)
> 

Thanks. I'm still not used to being able to push my own patches. The --- line
was never of any use before.

> > This fixes around 2000 piglit tests on BSW.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84777 (with many dupes)
> > Cc: "10.4" <mesa-stable at lists.freedesktop.org>
> 
> Cc: "10.4 10.3 10.2" <mesa-stable at lists.freedesktop.org>
> 

Is there a timeframe for getting this patch in? I would like to spend a few more
days working with the validation team to find out exactly why this works, since
they tell me it shouldn't fix anything. I don't think it hurts to move forward
with this patch, it fixes problems, but it's possible we'll end up with a
different fix after.

> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > 
> > ---
> >  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp  | 5 ++++-
> >  src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp | 3 ++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > index b353539..4f60797 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > @@ -534,8 +534,11 @@ vec4_generator::generate_gs_set_write_offset(struct brw_reg dst,
> >     brw_push_insn_state(p);
> >     brw_set_default_access_mode(p, BRW_ALIGN_1);
> >     brw_set_default_mask_control(p, BRW_MASK_DISABLE);
> > +   assert(src1.file == BRW_IMMEDIATE_VALUE &&
> > +          src1.type == BRW_REGISTER_TYPE_UD &&
> > +          src1.dw1.ud <= SHRT_MAX);
> 
> Don't you want USHRT_MAX?  (I don't think it'll matter in practice.)
> 
> >     brw_MUL(p, suboffset(stride(dst, 2, 2, 1), 3), stride(src0, 8, 2, 4),
> > -           src1);
> > +           retype(src1, BRW_REGISTER_TYPE_UW));
> 
> Please add:
> 
>    assert(brw->gen >= 7);
> 
> to this function, since UD * UW is illegal on Sandybridge:
> 
> "When multiplying a DW and a W, the W has to be on src0, and the DW has to be
>  on src1."
> 
> Which is fine, since Sandybridge doesn't run this code.  Gen7+ works like you
> expect, so I think this is a good change to make.
> 
> >     brw_set_default_access_mode(p, BRW_ALIGN_16);
> >     brw_pop_insn_state(p);
> >  }
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> > index db0e6cc..2d7ae5a 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> > @@ -388,7 +388,8 @@ vec4_gs_visitor::emit_control_data_bits()
> >            */
> >           src_reg per_slot_offset(this, glsl_type::uint_type);
> >           emit(SHR(dst_reg(per_slot_offset), dword_index, 2u));
> > -         emit(GS_OPCODE_SET_WRITE_OFFSET, mrf_reg, per_slot_offset, 1u);
> > +         emit(GS_OPCODE_SET_WRITE_OFFSET, mrf_reg, per_slot_offset,
> > +              src_reg(1u));
> 
> This hunk looks unnecessary (please drop it).
> 

This is C++ fail for me. I got lost when trying to figure out how the immediate
value is preserved, and understood how src_reg actually worked. I can certainly
drop this.

> >        }
> >  
> >        if (urb_write_flags & BRW_URB_WRITE_USE_CHANNEL_MASKS) {
> 
> With those changes,
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thanks.