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

[Kenneth Graunke]

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)

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

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

>        }
>  
>        if (urb_write_flags & BRW_URB_WRITE_USE_CHANNEL_MASKS) {

With those changes,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-stable/attachments/20141204/f332eaaa/attachment-0001.sig>