Add constant folding for new opcodes

Add constant folding for new opcodes

[Tobias Klausmann]

Hi,
please review the following 4 patches:

1b1cfc6 nvc0/ir: Handle OP_BFIND when folding constant expressions
d2d2727 nvc0/ir: Handle OP_POPCNT when folding constant expressions
86a1ee6 nvc0/ir: Handle reverse subop for OP_EXTBF when folding constant 
expressions
84563bf nvc0/ir: clear subop when folding constant expressions

src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 39 
+++++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)

Thanks,
Tobias Klausmann

[PATCH 1/4] nvc0/ir: clear subop when folding constant expressions

[Tobias Klausmann]

Some operations (e.g. OP_MUL/OP_MAD/OP_EXTBF might have a subop set.
After folding, make sure that it is cleared

Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
---
 src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
index 1a2c2e6..58092f4 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
@@ -563,6 +563,7 @@ ConstantFolding::expr(Instruction *i,
    } else {
       i->op = i->saturate ? OP_SAT : OP_MOV; /* SAT handled by unary() */
    }
+   i->subOp = 0;
 }
 
 void
-- 
1.8.4.5

[PATCH 2/4] nvc0/ir: Handle reverse subop for OP_EXTBF when folding constant expressions

[Tobias Klausmann]

Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
---
 src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
index 58092f4..93f7c2a 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
@@ -529,8 +529,18 @@ ConstantFolding::expr(Instruction *i,
          lshift = 32 - width - offset;
       }
       switch (i->dType) {
-      case TYPE_S32: res.data.s32 = (a->data.s32 << lshift) >> rshift; break;
-      case TYPE_U32: res.data.u32 = (a->data.u32 << lshift) >> rshift; break;
+      case TYPE_S32: {
+         res.data.s32 = (res.data.s32 << lshift) >> rshift;
+         if (i->subOp == NV50_IR_SUBOP_EXTBF_REV)
+            res.data.s32 = util_bitreverse(res.data.s32);
+         break;
+      }
+      case TYPE_U32: {
+         res.data.u32 = (res.data.u32 << lshift) >> rshift;
+         if (i->subOp == NV50_IR_SUBOP_EXTBF_REV)
+            res.data.u32 = util_bitreverse(res.data.u32);
+         break;
+      }
       default:
          return;
       }
-- 
1.8.4.5

Re: [PATCH 2/4] nvc0/ir: Handle reverse subop for OP_EXTBF when folding constant expressions

[Ilia Mirkin]

Can you verify that you tested how the HW handles this, as well as
exactly how you did it (i.e. how did you modify the code + piglit
test, what the results were, etc)

On Thu, May 29, 2014 at 3:43 PM, Tobias Klausmann
<tobias.johannes.klausmann@mni.thm.de> wrote:
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
> ---
>  src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> index 58092f4..93f7c2a 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> @@ -529,8 +529,18 @@ ConstantFolding::expr(Instruction *i,
>           lshift = 32 - width - offset;
>        }
>        switch (i->dType) {
> -      case TYPE_S32: res.data.s32 = (a->data.s32 << lshift) >> rshift; break;
> -      case TYPE_U32: res.data.u32 = (a->data.u32 << lshift) >> rshift; break;
> +      case TYPE_S32: {
> +         res.data.s32 = (res.data.s32 << lshift) >> rshift;
> +         if (i->subOp == NV50_IR_SUBOP_EXTBF_REV)
> +            res.data.s32 = util_bitreverse(res.data.s32);
> +         break;
> +      }
> +      case TYPE_U32: {
> +         res.data.u32 = (res.data.u32 << lshift) >> rshift;
> +         if (i->subOp == NV50_IR_SUBOP_EXTBF_REV)
> +            res.data.u32 = util_bitreverse(res.data.u32);
> +         break;
> +      }
>        default:
>           return;
>        }
> --
> 1.8.4.5
>

Re: [PATCH 2/4] nvc0/ir: Handle reverse subop for OP_EXTBF when folding constant expressions

[Tobias Klausmann]

Tested with:
MESA_EXTENSION_OVERRIDE=GL_ARB_gpu_shader5 ./shader_runner 
../tests/spec/arb_gpu_shader5/execution/built-in-functions/fs-bitfieldReverse.shader_test 
-> green output, so this should be ok

the test was not change though...


On 29.05.2014 21:47, Ilia Mirkin wrote:
> Can you verify that you tested how the HW handles this, as well as
> exactly how you did it (i.e. how did you modify the code + piglit
> test, what the results were, etc)
>
> On Thu, May 29, 2014 at 3:43 PM, Tobias Klausmann
> <tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org> wrote:
>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
>> ---
>>   src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>> index 58092f4..93f7c2a 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>> @@ -529,8 +529,18 @@ ConstantFolding::expr(Instruction *i,
>>            lshift = 32 - width - offset;
>>         }
>>         switch (i->dType) {
>> -      case TYPE_S32: res.data.s32 = (a->data.s32 << lshift) >> rshift; break;
>> -      case TYPE_U32: res.data.u32 = (a->data.u32 << lshift) >> rshift; break;
>> +      case TYPE_S32: {
>> +         res.data.s32 = (res.data.s32 << lshift) >> rshift;
>> +         if (i->subOp == NV50_IR_SUBOP_EXTBF_REV)
>> +            res.data.s32 = util_bitreverse(res.data.s32);
>> +         break;
>> +      }
>> +      case TYPE_U32: {
>> +         res.data.u32 = (res.data.u32 << lshift) >> rshift;
>> +         if (i->subOp == NV50_IR_SUBOP_EXTBF_REV)
>> +            res.data.u32 = util_bitreverse(res.data.u32);
>> +         break;
>> +      }
>>         default:
>>            return;
>>         }
>> --
>> 1.8.4.5
>>

Re: [PATCH 2/4] nvc0/ir: Handle reverse subop for OP_EXTBF when folding constant expressions

[Ilia Mirkin]

I think you misunderstood my suggestion. This code won't actually get
executed when running that test. What I was suggesting was to change
the shift/width argument passed to extbf when converting BREV, which
should in turn cause the test to start failing. The way in which you
need to modify the expected value the test generates will dictate
whether the bit reverse happens before or after the bitfield
extraction.

On Thu, May 29, 2014 at 3:53 PM, Tobias Klausmann
<tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org> wrote:
> Tested with:
> MESA_EXTENSION_OVERRIDE=GL_ARB_gpu_shader5 ./shader_runner
> ../tests/spec/arb_gpu_shader5/execution/built-in-functions/fs-bitfieldReverse.shader_test
> -> green output, so this should be ok
>
> the test was not change though...
>
>
>
> On 29.05.2014 21:47, Ilia Mirkin wrote:
>>
>> Can you verify that you tested how the HW handles this, as well as
>> exactly how you did it (i.e. how did you modify the code + piglit
>> test, what the results were, etc)
>>
>> On Thu, May 29, 2014 at 3:43 PM, Tobias Klausmann
>> <tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org> wrote:
>>>
>>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
>>> ---
>>>   src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 14
>>> ++++++++++++--
>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> index 58092f4..93f7c2a 100644
>>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> @@ -529,8 +529,18 @@ ConstantFolding::expr(Instruction *i,
>>>            lshift = 32 - width - offset;
>>>         }
>>>         switch (i->dType) {
>>> -      case TYPE_S32: res.data.s32 = (a->data.s32 << lshift) >> rshift;
>>> break;
>>> -      case TYPE_U32: res.data.u32 = (a->data.u32 << lshift) >> rshift;
>>> break;
>>> +      case TYPE_S32: {
>>> +         res.data.s32 = (res.data.s32 << lshift) >> rshift;
>>> +         if (i->subOp == NV50_IR_SUBOP_EXTBF_REV)
>>> +            res.data.s32 = util_bitreverse(res.data.s32);
>>> +         break;
>>> +      }
>>> +      case TYPE_U32: {
>>> +         res.data.u32 = (res.data.u32 << lshift) >> rshift;
>>> +         if (i->subOp == NV50_IR_SUBOP_EXTBF_REV)
>>> +            res.data.u32 = util_bitreverse(res.data.u32);
>>> +         break;
>>> +      }
>>>         default:
>>>            return;
>>>         }
>>> --
>>> 1.8.4.5
>>>
>

[PATCH 3/4] nvc0/ir: Handle OP_POPCNT when folding constant expressions

[Tobias Klausmann]

Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
---
 src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
index 93f7c2a..68b9a6d 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
@@ -546,6 +546,16 @@ ConstantFolding::expr(Instruction *i,
       }
       break;
    }
+   case OP_POPCNT: {
+      switch (i->dType) {
+      case TYPE_S32:
+      case TYPE_U32:
+         res.data.u32 = util_bitcount(a->data.u32 & b->data.u32); break;
+      default:
+        return;
+      }
+      break;
+   }
    default:
       return;
    }
-- 
1.8.4.5

Re: [PATCH 3/4] nvc0/ir: Handle OP_POPCNT when folding constant expressions

[Ilia Mirkin]

On Thu, May 29, 2014 at 3:43 PM, Tobias Klausmann
<tobias.johannes.klausmann@mni.thm.de> wrote:
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
> ---
>  src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> index 93f7c2a..68b9a6d 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> @@ -546,6 +546,16 @@ ConstantFolding::expr(Instruction *i,
>        }
>        break;
>     }
> +   case OP_POPCNT: {
> +      switch (i->dType) {
> +      case TYPE_S32:
> +      case TYPE_U32:
> +         res.data.u32 = util_bitcount(a->data.u32 & b->data.u32); break;
> +      default:
> +        return;
> +      }
> +      break;
> +   }

Why does the data type matter? I think you can handle this like AND is
handled -- just always do it.

Also, please add support for a single-argument version of POPCNT. This
will happen as a result of lowering for sm50 (maxwell), for which it's
a 1-arg instruction. Basically you need to add a case to opnd() which
checks that there's only one argument and does it accordingly. [The
existing function assumes a float op, and it'd be a pain to modify
it.]

>     default:
>        return;
>     }
> --
> 1.8.4.5
>

[PATCH 4/4] nvc0/ir: Handle OP_BFIND when folding constant expressions

[Tobias Klausmann]

Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
---
 src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
index 68b9a6d..a56756c 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
@@ -556,6 +556,20 @@ ConstantFolding::expr(Instruction *i,
       }
       break;
    }
+   case OP_BFIND: {
+      int shift = 0;
+      if (i->subOp == NV50_IR_SUBOP_BFIND_SAMT)
+         shift = 32 - (b->data.u32 & 0xff);
+      switch (i->dType) {
+      case TYPE_S32:
+         res.data.s32 = util_last_bit_signed(a->data.s32 >> shift)- 1; break;
+      case TYPE_U32:
+         res.data.u32 = util_last_bit(a->data.u32 >> shift) -1; break;
+      default:
+         return;
+      }
+      break;
+   }
    default:
       return;
    }
-- 
1.8.4.5

Re: [PATCH 4/4] nvc0/ir: Handle OP_BFIND when folding constant expressions

[Ilia Mirkin]

How did you test this? I'm like 99% sure it's wrong. (But I'm avoiding
saying how it's wrong so that you don't just fix it to match what I
say, but instead work it out yourself by doing the tests that you
should have been doing in the first place. Or prove me wrong.)

On Thu, May 29, 2014 at 3:43 PM, Tobias Klausmann
<tobias.johannes.klausmann@mni.thm.de> wrote:
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
> ---
>  src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> index 68b9a6d..a56756c 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> @@ -556,6 +556,20 @@ ConstantFolding::expr(Instruction *i,
>        }
>        break;
>     }
> +   case OP_BFIND: {
> +      int shift = 0;
> +      if (i->subOp == NV50_IR_SUBOP_BFIND_SAMT)
> +         shift = 32 - (b->data.u32 & 0xff);
> +      switch (i->dType) {
> +      case TYPE_S32:
> +         res.data.s32 = util_last_bit_signed(a->data.s32 >> shift)- 1; break;
> +      case TYPE_U32:
> +         res.data.u32 = util_last_bit(a->data.u32 >> shift) -1; break;
> +      default:
> +         return;
> +      }
> +      break;
> +   }
>     default:
>        return;
>     }
> --
> 1.8.4.5
>