Re: [Mesa-dev] [PATCH] gm107/ir: fix sign bit emission for FADD32I

2016-07-04 Thread Ilia Mirkin 
Oh wait, we don't fold it in because it's a SUB, duh. So that bit makes
sense. I'd slightly prefer flipping the neg modifier, but your call.

On Jul 4, 2016 8:17 AM, "Samuel Pitoiset"  wrote:

>
>
> On 07/04/2016 01:59 PM, Ilia Mirkin wrote:
>
>> That flips the sign of the immediate. Why not flip 0x35, which is an
>> explicit neg modifier? I guess we mess with the immediate in the other
>> emitters, so r-b either way.
>>
>
> Sure, that flips the sign yeah.
>
> I guess we mess up with the neg modifier actually because it is not set
> for that source. I didn't check the other emitters but that test passes on
> GK107 (and probably on GF100/GK110 as well).
>
>
>> As an aside, how did you hit this? Should have gotten folded in...
>>
>
> With piglit. :)
>
>
>>
>> On Jul 4, 2016 7:12 AM, "Samuel Pitoiset" > > wrote:
>>
>> When emitting OP_SUB, the sign bit for FADD and FADD32I is not
>> at the same position. It's at position 45 for FADD but 51 for FADD32I.
>>
>> This fixes the following piglit test:
>> tests/spec/arb_fragment_program/fdo30337b.shader_test
>>
>> Signed-off-by: Samuel Pitoiset > >
>> Cc: > >
>> ---
>>  src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp | 9
>> ++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git
>> a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
>> index 2c5e8f6..f1ba27a 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
>> @@ -1234,6 +1234,9 @@ CodeEmitterGM107::emitFADD()
>>emitABS(0x2e, insn->src(0));
>>emitNEG(0x2d, insn->src(1));
>>emitFMZ(0x2c, 1);
>> +
>> +  if (insn->op == OP_SUB)
>> + code[1] ^= 0x2000;
>> } else {
>>emitInsn(0x0800);
>>emitABS(0x39, insn->src(1));
>> @@ -1243,10 +1246,10 @@ CodeEmitterGM107::emitFADD()
>>emitNEG(0x35, insn->src(1));
>>emitCC  (0x34);
>>emitIMMD(0x14, 32, insn->src(1));
>> -   }
>>
>> -   if (insn->op == OP_SUB)
>> -  code[1] ^= 0x2000;
>> +  if (insn->op == OP_SUB)
>> + code[1] ^= 0x0008;
>> +   }
>>
>> emitGPR(0x08, insn->src(0));
>> emitGPR(0x00, insn->def(0));
>> --
>> 2.8.0
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org > >
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
> --
> -Samuel
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gm107/ir: fix sign bit emission for FADD32I

2016-07-04 Thread Samuel Pitoiset 



On 07/04/2016 01:59 PM, Ilia Mirkin wrote:

That flips the sign of the immediate. Why not flip 0x35, which is an
explicit neg modifier? I guess we mess with the immediate in the other
emitters, so r-b either way.


Sure, that flips the sign yeah.

I guess we mess up with the neg modifier actually because it is not set 
for that source. I didn't check the other emitters but that test passes 
on GK107 (and probably on GF100/GK110 as well).




As an aside, how did you hit this? Should have gotten folded in...


With piglit. :)




On Jul 4, 2016 7:12 AM, "Samuel Pitoiset" mailto:samuel.pitoi...@gmail.com>> wrote:

When emitting OP_SUB, the sign bit for FADD and FADD32I is not
at the same position. It's at position 45 for FADD but 51 for FADD32I.

This fixes the following piglit test:
tests/spec/arb_fragment_program/fdo30337b.shader_test

Signed-off-by: Samuel Pitoiset mailto:samuel.pitoi...@gmail.com>>
Cc: mailto:mesa-sta...@lists.freedesktop.org>>
---
 src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp | 9
++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git
a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
index 2c5e8f6..f1ba27a 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
@@ -1234,6 +1234,9 @@ CodeEmitterGM107::emitFADD()
   emitABS(0x2e, insn->src(0));
   emitNEG(0x2d, insn->src(1));
   emitFMZ(0x2c, 1);
+
+  if (insn->op == OP_SUB)
+ code[1] ^= 0x2000;
} else {
   emitInsn(0x0800);
   emitABS(0x39, insn->src(1));
@@ -1243,10 +1246,10 @@ CodeEmitterGM107::emitFADD()
   emitNEG(0x35, insn->src(1));
   emitCC  (0x34);
   emitIMMD(0x14, 32, insn->src(1));
-   }

-   if (insn->op == OP_SUB)
-  code[1] ^= 0x2000;
+  if (insn->op == OP_SUB)
+ code[1] ^= 0x0008;
+   }

emitGPR(0x08, insn->src(0));
emitGPR(0x00, insn->def(0));
--
2.8.0

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



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

Re: [Mesa-dev] [PATCH] gm107/ir: fix sign bit emission for FADD32I

2016-07-04 Thread Ilia Mirkin 
That flips the sign of the immediate. Why not flip 0x35, which is an
explicit neg modifier? I guess we mess with the immediate in the other
emitters, so r-b either way.

As an aside, how did you hit this? Should have gotten folded in...

On Jul 4, 2016 7:12 AM, "Samuel Pitoiset"  wrote:

> When emitting OP_SUB, the sign bit for FADD and FADD32I is not
> at the same position. It's at position 45 for FADD but 51 for FADD32I.
>
> This fixes the following piglit test:
> tests/spec/arb_fragment_program/fdo30337b.shader_test
>
> Signed-off-by: Samuel Pitoiset 
> Cc: 
> ---
>  src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> index 2c5e8f6..f1ba27a 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> @@ -1234,6 +1234,9 @@ CodeEmitterGM107::emitFADD()
>emitABS(0x2e, insn->src(0));
>emitNEG(0x2d, insn->src(1));
>emitFMZ(0x2c, 1);
> +
> +  if (insn->op == OP_SUB)
> + code[1] ^= 0x2000;
> } else {
>emitInsn(0x0800);
>emitABS(0x39, insn->src(1));
> @@ -1243,10 +1246,10 @@ CodeEmitterGM107::emitFADD()
>emitNEG(0x35, insn->src(1));
>emitCC  (0x34);
>emitIMMD(0x14, 32, insn->src(1));
> -   }
>
> -   if (insn->op == OP_SUB)
> -  code[1] ^= 0x2000;
> +  if (insn->op == OP_SUB)
> + code[1] ^= 0x0008;
> +   }
>
> emitGPR(0x08, insn->src(0));
> emitGPR(0x00, insn->def(0));
> --
> 2.8.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev