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

[Matt Turner]

On Fri, Mar 13, 2015 at 10:35 AM, Matt Turner <mattst88 at gmail.com> wrote:
> 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!

Ken was telling me today that he independently ran into this and had
some different idea about fixing it. Adding him to Cc.