[Mesa-dev] [PATCH v2] nvc0/ir: be more careful about preserving modifiers in SHLADD creation

[Samuel Pitoiset]

On 10/13/2016 03:56 PM, Ilia Mirkin wrote:
> On Thu, Oct 13, 2016 at 9:53 AM, Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
>>
>>
>> On 10/12/2016 08:42 PM, Ilia Mirkin wrote:
>>>
>>> src2 was being given the wrong modifier, and we were not properly
>>> managing the modifier on the SHL source either.
>>>
>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>> ---
>>>  src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 12
>>> +++++-------
>>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> index d88bb34..737bda3 100644
>>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> @@ -2163,7 +2163,6 @@ LateAlgebraicOpt::tryADDToSHLADD(Instruction *add)
>>>     Value *src1 = add->getSrc(1);
>>>     ImmediateValue imm;
>>>     Instruction *shl;
>>> -   Modifier mod[2];
>>>     Value *src;
>>>     int s;
>>>
>>> @@ -2182,20 +2181,19 @@ LateAlgebraicOpt::tryADDToSHLADD(Instruction *add)
>>>     src = add->getSrc(s);
>>>     shl = src->getUniqueInsn();
>>>
>>> -   if (shl->bb != add->bb || shl->usesFlags() || shl->subOp)
>>> +   if (shl->bb != add->bb || shl->usesFlags() || shl->subOp ||
>>> shl->src(0).mod)
>>
>>
>> SHL can't have any modifiers, why do you check it here? I would say it's
>> just for safety, but if the compiler adds modifiers to SHL something is
>> really wrong...
>
> That's a target-specific property. There could be some future
> hypothetical target that allows modifiers on shl. The check is cheap,
> so may as well :) If it were a more expensive check, I'd do it in a
> #if debug thing maybe.

Yeah, hypothetical :)

>
>>
>> Looks good now.
>>
>> Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>
> Thanks!
>
>>
>>
>>>        return false;
>>>
>>>     if (!shl->src(1).getImmediate(imm))
>>>        return false;
>>>
>>> -   mod[0] = add->src(0).mod;
>>> -   mod[1] = add->src(1).mod;
>>> -
>>>     add->op = OP_SHLADD;
>>>     add->setSrc(2, add->src(!s));
>>> -   add->src(2).mod = mod[s];
>>> -
>>> +   // SHL can't have any modifiers, but the ADD source may have had
>>> +   // one. Preserve it.
>>>     add->setSrc(0, shl->getSrc(0));
>>> +   if (s == 1)
>>> +      add->src(0).mod = add->src(1).mod;
>>>     add->setSrc(1, new_ImmediateValue(shl->bb->getProgram(),
>>> imm.reg.data.u32));
>>>     add->src(1).mod = Modifier(0);
>>>
>>>
>>
>> --
>> -Samuel

-- 
-Samuel