[Mesa-dev] [PATCH] i965: in MAD->MUL, switch last argument to be immediate

[Kenneth Graunke]

On Friday, March 13, 2015 03:20:27 PM Tapani Pälli wrote:
> Commit bb33a31 introduced MAD->MUL optimization which did not
> take account case where one of the remaining sources is immediate
> and did not report progress. Patch changes last one of the sources
> to be immediate and adds progress reporting. If both are, this is
> taken care of by the same opt_algebraic pass on later run.
> 
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89569
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 8702ea8..710a7e6 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -2490,7 +2490,14 @@ fs_visitor::opt_algebraic()
>           } else if (inst->src[0].is_zero()) {
>              inst->opcode = BRW_OPCODE_MUL;
>              inst->src[0] = inst->src[2];
> +            /* Last one needs to be immediate. */
> +            if (inst->src[0].file == IMM) {
> +               inst->src[2] = inst->src[1];
> +               inst->src[1] = inst->src[0];
> +               inst->src[0] = inst->src[2];
> +            }
>              inst->src[2] = reg_undef;
> +            progress = true;

(in case Matt decides to go with these patches, here are review comments)

Please use a temporary register instead of inst->src[2]:

   fs_reg tmp = inst->src[1];
   inst->src[1] = inst->src[0];
   inst->src[0] = temp;

This avoids leaving cruft in inst->src[2], which probably won't break
anything, but seems bad.  Ideally, inst->src[] at/beyond inst->sources
would stay set to BAD_FILE (reg_undef).

Today, Matt always allocates the src[] array to be at least 3 large,
but conceivably we could stop doing that for unary/binary operations,
so accessing src[] beyond inst->sources could be invalid.

Good catch!  You definitely need to handle this for the other two cases
as well - the game "Tiny and Big: Grandpa's Leftovers" contains fragment
programs that result in "ADD dst 1.0 src" (the other cases).

You could probably just check if you need to commute after the switch
statement, so you don't have to duplicate the check and swap in each
of the three cases.   The is_commutative() method I created in my other
series would probably help with that.

>           } else if (inst->src[1].is_one()) {
>              inst->opcode = BRW_OPCODE_ADD;
>              inst->src[1] = inst->src[2];
-------------- 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-dev/attachments/20150313/fddc308b/attachment.sig>