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

[Matt Turner]

On Fri, Mar 13, 2015 at 6:20 AM, Tapani Pälli <tapani.palli at intel.com> 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];
> +            }

To confirm, we had a MAD dst, 0.0, x, y, and we transform it here into
MUL dst, x, y. But x is actually an immediate and since we didn't
report progress (and nothing else did in the optimization loop), we
attempt to make an illegal instruction.

Nice find!

I might add something like this to the commit message.

Doesn't this problem exist for other cases too? The next two, checking
for 1.0 multiplicand arguments, seem to have the same problem.

Could you fix those at the same time?

Thanks Tapani & Curro!