---------- Forwarded message ----------
From: Ivan Kalvachev <ikalvac...@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 <nhaeh...@gmail.com>
On 10/26/15, Nicolai Hähnle <nhaeh...@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.
>> Signed-off-by: Ivan Kalvachev <ikalvac...@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
From 341aa63bf528ae4118ff92cefe890528709b863f Mon Sep 17 00:00:00 2001
From: Ivan Kalvachev <ikalvac...@gmail.com>
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 <ikalvac...@gmail.com>
---
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 0xBF800000: /* -1.0f */
*sel = V_SQ_ALU_SRC_1;
- *neg ^= 1;
+ *neg ^= !abs;
break;
case 0xBF000000: /* -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,
- &nalu->src[i].sel, &nalu->src[i].neg);
+ &nalu->src[i].sel, &nalu->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, unsigned *neg);
+ unsigned *sel, unsigned *neg, unsigned abs);
void r600_bytecode_disasm(struct r600_bytecode *bc);
void r600_bytecode_alu_read(struct r600_bytecode *bc,
struct r600_bytecode_alu *alu, uint32_t word0, uint32_t word1);
diff --git a/src/gallium/drivers/r600/r600_shader.c b/src/gallium/drivers/r600/r600_shader.c
index 8efe902..50c0329 100644
--- a/src/gallium/drivers/r600/r600_shader.c
+++ b/src/gallium/drivers/r600/r600_shader.c
@@ -1008,7 +1008,7 @@ static void tgsi_src(struct r600_shader_ctx *ctx,
(tgsi_src->Register.SwizzleX == tgsi_src->Register.SwizzleW)) {
index = tgsi_src->Register.Index * 4 + tgsi_src->Register.SwizzleX;
- r600_bytecode_special_constants(ctx->literals[index], &r600_src->sel, &r600_src->neg);
+ r600_bytecode_special_constants(ctx->literals[index], &r600_src->sel, &r600_src->neg, r600_src->abs);
if (r600_src->sel != V_SQ_ALU_SRC_LITERAL)
return;
}
--
2.5.1
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
