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

[Emil Velikov]

On 29 October 2015 at 09:24, Ivan Kalvachev  wrote:
> On 10/29/15, Ilia Mirkin  wrote:
>> On Wed, Oct 28, 2015 at 8:52 PM, Ivan Kalvachev 
>> wrote:
>>> I'm attaching v3 of the patch. Same as v2, but without the extra empty
>>> line.
>>
>> FYI, there's a lot of overhead to reviewing an attached patch (in
>> fact, gmail makes it extra difficult since you can't even see the
>> thing in the name of protection from the unknown). Please use 'git
>> send-email' (or equivalent mechanism) to make the patches appear
>> inline.
>>
>> Cheers,
>>
>>   -ilia
>
> Sorry about that.
> I did inline my v1 patch, to easy the review.
>
> Configuring smtp server is too much hassle for a single patch and I
> would like to avoid writing my email credentials if possible.
> If I'm about to send more patches, then I guess I would have to do that.
>
> I suspect that it is not possible to send mail directly to
> lists.freedesktop.org mail server, is it?
>
Setting things up it's as trivial as adding the following lines to
your $HOME/.config/git/config

[sendemail]
   smtpuser = ikalvac...@gmail.com
   smtpencryption = tls
   smtpserverport = 587
   smtpserver = smtp.gmail.com

For ease of use you can also set the TO address in
$(mesa_top)/.git/config. You can also move the above setup in here.

[sendemail]
   to = mesa-dev@lists.freedesktop.org


-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Felix Schwarz]

Am 29.10.2015 um 10:24 schrieb Ivan Kalvachev:
> Configuring smtp server is too much hassle for a single patch and I
> would like to avoid writing my email credentials if possible.
> If I'm about to send more patches, then I guess I would have to do that.

You might want to look into "git imap-send" and/or "git format-patch --stdout"
so git can generate emails which you can send with an email client like
thunderbird. That way you also get a nice way to "preview" the
messages/patches you are going to send.

Felix

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
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 

On 10/26/15, Nicolai Hähnle  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 

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 
---
   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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ivan Kalvachev]

On 10/29/15, Ilia Mirkin  wrote:
> On Wed, Oct 28, 2015 at 8:52 PM, Ivan Kalvachev 
> wrote:
>> I'm attaching v3 of the patch. Same as v2, but without the extra empty
>> line.
>
> FYI, there's a lot of overhead to reviewing an attached patch (in
> fact, gmail makes it extra difficult since you can't even see the
> thing in the name of protection from the unknown). Please use 'git
> send-email' (or equivalent mechanism) to make the patches appear
> inline.
>
> Cheers,
>
>   -ilia

Sorry about that.
I did inline my v1 patch, to easy the review.

Configuring smtp server is too much hassle for a single patch and I
would like to avoid writing my email credentials if possible.
If I'm about to send more patches, then I guess I would have to do that.

I suspect that it is not possible to send mail directly to
lists.freedesktop.org mail server, is it?


On 10/29/15, Nicolai Hähnle  wrote:
> On 29.10.2015 01:52, Ivan Kalvachev wrote:
>> -- Forwarded message --
>> From: Ivan Kalvachev 
>> 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 
>>
>> On 10/26/15, Nicolai Hähnle  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 
>
> This should probably also go to stable.
>
> Do you need somebody to push this for you or can you do it yourself?
>
> Cheers,
> Nicolai

Yes, please.
I'm not developer and I cannot push it myself.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ivan Kalvachev]

-- Forwarded message --
From: Ivan Kalvachev 
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 

On 10/26/15, Nicolai Hähnle  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.

>> Signed-off-by: Ivan Kalvachev 
>> ---
>>   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
From 341aa63bf528ae4118ff92cefe890528709b863f Mon Sep 17 00:00:00 2001
From: Ivan Kalvachev 
Date: Sun, 25 Oct 2015 01:16:58 +0300
Subject: [PATCH] r600g: Fix special negative immediate constants when using
 ABS modifier.

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.

Signed-off-by: Ivan Kalvachev 
---
 src/gallium/drivers/r600/r600_asm.c| 8 
 src/gallium/drivers/r600/r600_asm.h| 2 +-
 src/gallium/drivers/r600/r600_shader.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/gallium/drivers/r600/r600_asm.c b/src/gallium/drivers/r600/r600_asm.c
index bc69806..ee7beee 100644
--- a/src/gallium/drivers/r600/r600_asm.c
+++ b/src/gallium/drivers/r600/r600_asm.c
@@ -635,7 +635,7 @@ 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)
 {
 	switch(value) {
 	case 0:
@@ -655,11 +655,11 @@ void r600_bytecode_special_constants(uint32_t value, unsigned *sel, unsigned *ne
 		break;
 	case 0xBF80: /* -1.0f */
 		*sel = V_SQ_ALU_SRC_1;
-		*neg ^= 1;
+		*neg ^= !abs;
 		break;
 	case 0xBF00: /* -0.5f */
 		*sel = V_SQ_ALU_SRC_0_5;
-		*neg ^= 1;
+		*neg ^= !abs;
 		break;
 	default:
 		*sel = V_SQ_ALU_SRC_LITERAL;
@@ -1208,7 +1208,7 @@ int r600_bytecode_add_alu_type(struct r600_bytecode *bc,
 		}
 		if (nalu->src[i].sel == V_SQ_ALU_SRC_LITERAL)
 			r600_bytecode_special_constants(nalu->src[i].value,
->src[i].sel, >src[i].neg);
+>src[i].sel, >src[i].neg, nalu->src[i].abs);
 	}
 	if (nalu->dst.sel >= bc->ngpr) {
 		bc->ngpr = nalu->dst.sel + 1;
diff --git a/src/gallium/drivers/r600/r600_asm.h b/src/gallium/drivers/r600/r600_asm.h
index 7cf3a09..d48ad1e 100644
--- a/src/gallium/drivers/r600/r600_asm.h
+++ b/src/gallium/drivers/r600/r600_asm.h
@@ -255,7 +255,7 @@ int r600_bytecode_add_cfinst(struct r600_bytecode *bc,
 int r600_bytecode_add_alu_type(struct r600_bytecode *bc,
 		const struct r600_bytecode_alu *alu, unsigned type);
 void r600_bytecode_special_constants(uint32_t value,
-		unsigned *sel, 

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

[Ilia Mirkin]

On Wed, Oct 28, 2015 at 8:52 PM, Ivan Kalvachev  wrote:
> I'm attaching v3 of the patch. Same as v2, but without the extra empty line.

FYI, there's a lot of overhead to reviewing an attached patch (in
fact, gmail makes it extra difficult since you can't even see the
thing in the name of protection from the unknown). Please use 'git
send-email' (or equivalent mechanism) to make the patches appear
inline.

Cheers,

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev