[Mesa-dev] [PATCH v3] r600g: Fix special negative immediate constants when using ABS modifier.

[Nicolai Hähnle]

On 29.10.2015 01:52, Ivan Kalvachev wrote:
> ---------- Forwarded message ----------
> From: Ivan Kalvachev <ikalvachev at gmail.com>
> Date: Wed, 28 Oct 2015 23:46:44 +0200
> Subject: [PATCH v3] r600g: Fix special negative immediate constants
> when using ABS modifier.
> To: Nicolai Hähnle <nhaehnle at gmail.com>
>
> On 10/26/15, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>> Hi Ivan,
>>
>> On 25.10.2015 02:00, Ivan Kalvachev wrote:
>>> Some constants (like 1.0 and 0.5) could be inlined as immediate inputs
>>> without using their literal value. The r600_bytecode_special_constants()
>>> function emulates the negative of these constants by using NEG modifier.
>>>
>>> However some shaders define -1.0 constant and want to use it as 1.0.
>>> They do so by using ABS modifier. But r600_bytecode_special_constants()
>>> set NEG in addition to ABS. Since NEG modifier have priority over ABS
>>> one,
>>> we get -|1.0| as result, instead of |1.0|.
>>>
>>> The patch simply prevents the additional switching of NEG when ABS is
>>> set.
>>
>> Nice catch. Is there a simple test case (e.g. in piglit) that exposes
>> the incorrect behavior?
>
> Not that I know of.
>
> I've located the bug investigating visual problem in Nine.
> https://github.com/iXit/Mesa-3D/issues/126
> https://github.com/iXit/Mesa-3D/issues/127
>
> I also heard that it fixes artifacts in "Need for Speed: Undercover"
> and "Skyrim", once again, when using Nine.

I see. I guess it's not too surprising that Nine creates shaders that 
look a bit different from the Mesa statetracker's.

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

This should probably also go to stable.

Do you need somebody to push this for you or can you do it yourself?

Cheers,
Nicolai

>>> Signed-off-by: Ivan Kalvachev <ikalvachev at gmail.com>
>>> ---
>>>    src/gallium/drivers/r600/r600_asm.c    | 9 +++++----
>>>    src/gallium/drivers/r600/r600_shader.c | 2 +-
>>>    2 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/r600/r600_asm.c
>>> b/src/gallium/drivers/r600/r600_asm.c
>>> index bc69806..8fc622c 100644
>>> --- a/src/gallium/drivers/r600/r600_asm.c
>>> +++ b/src/gallium/drivers/r600/r600_asm.c
>>> @@ -635,8 +635,9 @@ static int replace_gpr_with_pv_ps(struct r600_bytecode
>>> *bc,
>>>           return 0;
>>>    }
>>>
>>> -void r600_bytecode_special_constants(uint32_t value, unsigned *sel,
>>> unsigned *neg)
>>> +void r600_bytecode_special_constants(uint32_t value, unsigned *sel,
>>> unsigned *neg, unsigned abs)
>>>    {
>>> +
>>
>> Please remove the extra whitespace line.
>>
>> Cheers,
>> Nicolai
>>
>
> I'm attaching v3 of the patch. Same as v2, but without the extra empty line.
>
> Best Regards
>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>