[Mesa-dev] [PATCH] i965/fs: Fix 32-bit integer multiplication.

[Kenneth Graunke]

On 08/15/2011 10:57 PM, Eric Anholt wrote:
> The MUL opcode does a 16bit * 32bit multiply, and we need to do the
> MACH to get the top 16bit * 32bit added in.
> 
> Fixes fs-op-mult-int-*, fs-op-mult-ivec*

Sigh.  You're right, of course...this is straight out of the
documentation (aside from dropping the high 64-bits).

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
(assuming it's passed a full piglit run)

To implement a first stab at your "Emit just the MUL if we know an
operand is small enough" FINISHME, we could at least check if it's an
ir_constant of integer type that fits in 16-bits.  That would be easy
and at least make things like 2*x not suck.

But now I'm a bit paranoid...are we following the other restrictions?

- Do we generate multiplies with mixed float/integer operands?
  I could see it happen, say, after some optimization pass.
- Do we use saturate on integers?
  Doubtful, as it doesn't make much sense, but...

Thankfully, we never use W and UW types (no 16-bit integers), so...we're
okay on those.

Also, using the accumulator always makes me paranoid, wondering exactly
when and what we're accumulating.  Do MADs on integers work, given that
we currently implement MAD via MAC?

> ---
>  src/mesa/drivers/dri/i965/brw_fs_emit.cpp    |    5 +++++
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp |   18 +++++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> index e168e54..0a4b236 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> @@ -655,6 +655,11 @@ fs_visitor::generate_code()
>        case BRW_OPCODE_MUL:
>  	 brw_MUL(p, dst, src[0], src[1]);
>  	 break;
> +      case BRW_OPCODE_MACH:
> +	 brw_set_acc_write_control(p, 1);
> +	 brw_MACH(p, dst, src[0], src[1]);
> +	 brw_set_acc_write_control(p, 0);
> +	 break;
>  
>        case BRW_OPCODE_FRC:
>  	 brw_FRC(p, dst, src[0]);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index 2e3f9be..13ff840 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -287,7 +287,23 @@ fs_visitor::visit(ir_expression *ir)
>        break;
>  
>     case ir_binop_mul:
> -      emit(BRW_OPCODE_MUL, this->result, op[0], op[1]);
> +      if (ir->type->is_integer()) {
> +	 /* For integer multiplication, the MUL uses the low 16 bits
> +	  * of one of the operands (src0 on gen6, src1 on gen7).  The
> +	  * MACH accumulates in the contribution of the upper 16 bits
> +	  * of that operand.
> +	  *
> +	  * FINISHME: Emit just the MUL if we know an operand is small
> +	  * enough.
> +	  */
> +	 struct brw_reg acc = retype(brw_acc_reg(), BRW_REGISTER_TYPE_D);
> +
> +	 emit(BRW_OPCODE_MUL, acc, op[0], op[1]);
> +	 emit(BRW_OPCODE_MACH, reg_null_d, op[0], op[1]);
> +	 emit(BRW_OPCODE_MOV, this->result, fs_reg(acc));
> +      } else {
> +	 emit(BRW_OPCODE_MUL, this->result, op[0], op[1]);
> +      }
>        break;
>     case ir_binop_div:
>        assert(!"not reached: should be handled by ir_div_to_mul_rcp");