Re: [Mesa-dev] [PATCH 03/20] nir: Support lowering vote intrinsics

[Matt Turner]

On Tue, Jul 18, 2017 at 1:34 PM, Connor Abbott  wrote:
> On Mon, Jul 10, 2017 at 10:18 AM, Matt Turner  wrote:
>> On Thu, Jul 6, 2017 at 8:04 PM, Connor Abbott  wrote:
>>> On Thu, Jul 6, 2017 at 4:48 PM, Matt Turner  wrote:
 ... trivially (as allowed by the spec!) by reusing the existing
 nir_opt_intrinsics code.
 ---
  src/compiler/nir/nir.h| 4 
  src/compiler/nir/nir_opt_intrinsics.c | 6 +++---
  2 files changed, 7 insertions(+), 3 deletions(-)

 diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
 index 44a1d0887e..401c41f155 100644
 --- a/src/compiler/nir/nir.h
 +++ b/src/compiler/nir/nir.h
 @@ -1821,6 +1821,10 @@ typedef struct nir_shader_compiler_options {
 bool lower_extract_byte;
 bool lower_extract_word;

 +   bool lower_vote_any;
 +   bool lower_vote_all;
 +   bool lower_vote_eq;
>>>
>>> Since there are potentially multiple ways to lower these (voteAny(x)
>>> -> !voteAll(!x), using ballotARB(), etc.), and the way they're lowered
>>> is a little... unexpected (although admittedly legal!), why don't we
>>> use a more descriptive name, like lower_vote_*_trivial? While we're at
>>> it, I highly doubt that an implementation would want this kind of
>>> lowering for just one of the intrinsics, so we can merge this into a
>>> single flag, say lower_vote_trivial.
>>
>> Thanks, both good ideas. I've replaced all three fields with a
>> lower_vote_trivial field.
>
> I had a closer look at your branch with the updated patch, and the
> logic here, repeated in two places, seems backwards:
>
> if (!val || b.shader->options->lower_vote_trivial)
>continue;
>
> This will skip processing the instruction at all if you set
> lower_vote_trivial, even if val is non-NULL, which seems like the
> opposite of what you want. Also, even once you fix this:
>
> if (!val && !b.shader->options->lower_vote_trivial)
>continue;

Indeed. Thanks.

>
> You'll still segfault in the vote_any/vote_all case if the source
> isn't constant, since you'll try to dereference val when it doesn't
> exist. You can fix this by changing the line below to:
>
> replacement = nir_ssa_for_src(, instr->src[0], 1);

Needs to be s/instr/intrin/, but yeah, good catch :)

> in the previous patch. I'm kinda nervous that lower_vote_trivial seems
> untested, since it never would've worked as-is, but I can't see any
> other problems so patches 2 & 3 get my R-b with these fixes. But you
> might want to write some really simple vertex shader piglit tests,
> even if you only use dynamically uniform arguments, to make sure this
> is working correctly.

Done. Tests will be on the piglit list shortly.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 03/20] nir: Support lowering vote intrinsics

[Connor Abbott]

On Mon, Jul 10, 2017 at 10:18 AM, Matt Turner  wrote:
> On Thu, Jul 6, 2017 at 8:04 PM, Connor Abbott  wrote:
>> On Thu, Jul 6, 2017 at 4:48 PM, Matt Turner  wrote:
>>> ... trivially (as allowed by the spec!) by reusing the existing
>>> nir_opt_intrinsics code.
>>> ---
>>>  src/compiler/nir/nir.h| 4 
>>>  src/compiler/nir/nir_opt_intrinsics.c | 6 +++---
>>>  2 files changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>>> index 44a1d0887e..401c41f155 100644
>>> --- a/src/compiler/nir/nir.h
>>> +++ b/src/compiler/nir/nir.h
>>> @@ -1821,6 +1821,10 @@ typedef struct nir_shader_compiler_options {
>>> bool lower_extract_byte;
>>> bool lower_extract_word;
>>>
>>> +   bool lower_vote_any;
>>> +   bool lower_vote_all;
>>> +   bool lower_vote_eq;
>>
>> Since there are potentially multiple ways to lower these (voteAny(x)
>> -> !voteAll(!x), using ballotARB(), etc.), and the way they're lowered
>> is a little... unexpected (although admittedly legal!), why don't we
>> use a more descriptive name, like lower_vote_*_trivial? While we're at
>> it, I highly doubt that an implementation would want this kind of
>> lowering for just one of the intrinsics, so we can merge this into a
>> single flag, say lower_vote_trivial.
>
> Thanks, both good ideas. I've replaced all three fields with a
> lower_vote_trivial field.

I had a closer look at your branch with the updated patch, and the
logic here, repeated in two places, seems backwards:

if (!val || b.shader->options->lower_vote_trivial)
   continue;

This will skip processing the instruction at all if you set
lower_vote_trivial, even if val is non-NULL, which seems like the
opposite of what you want. Also, even once you fix this:

if (!val && !b.shader->options->lower_vote_trivial)
   continue;

You'll still segfault in the vote_any/vote_all case if the source
isn't constant, since you'll try to dereference val when it doesn't
exist. You can fix this by changing the line below to:

replacement = nir_ssa_for_src(, instr->src[0], 1);

in the previous patch. I'm kinda nervous that lower_vote_trivial seems
untested, since it never would've worked as-is, but I can't see any
other problems so patches 2 & 3 get my R-b with these fixes. But you
might want to write some really simple vertex shader piglit tests,
even if you only use dynamically uniform arguments, to make sure this
is working correctly.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 03/20] nir: Support lowering vote intrinsics

[Matt Turner]

On Thu, Jul 6, 2017 at 8:04 PM, Connor Abbott  wrote:
> On Thu, Jul 6, 2017 at 4:48 PM, Matt Turner  wrote:
>> ... trivially (as allowed by the spec!) by reusing the existing
>> nir_opt_intrinsics code.
>> ---
>>  src/compiler/nir/nir.h| 4 
>>  src/compiler/nir/nir_opt_intrinsics.c | 6 +++---
>>  2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>> index 44a1d0887e..401c41f155 100644
>> --- a/src/compiler/nir/nir.h
>> +++ b/src/compiler/nir/nir.h
>> @@ -1821,6 +1821,10 @@ typedef struct nir_shader_compiler_options {
>> bool lower_extract_byte;
>> bool lower_extract_word;
>>
>> +   bool lower_vote_any;
>> +   bool lower_vote_all;
>> +   bool lower_vote_eq;
>
> Since there are potentially multiple ways to lower these (voteAny(x)
> -> !voteAll(!x), using ballotARB(), etc.), and the way they're lowered
> is a little... unexpected (although admittedly legal!), why don't we
> use a more descriptive name, like lower_vote_*_trivial? While we're at
> it, I highly doubt that an implementation would want this kind of
> lowering for just one of the intrinsics, so we can merge this into a
> single flag, say lower_vote_trivial.

Thanks, both good ideas. I've replaced all three fields with a
lower_vote_trivial field.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 03/20] nir: Support lowering vote intrinsics

[Connor Abbott]

On Thu, Jul 6, 2017 at 4:48 PM, Matt Turner  wrote:
> ... trivially (as allowed by the spec!) by reusing the existing
> nir_opt_intrinsics code.
> ---
>  src/compiler/nir/nir.h| 4 
>  src/compiler/nir/nir_opt_intrinsics.c | 6 +++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index 44a1d0887e..401c41f155 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -1821,6 +1821,10 @@ typedef struct nir_shader_compiler_options {
> bool lower_extract_byte;
> bool lower_extract_word;
>
> +   bool lower_vote_any;
> +   bool lower_vote_all;
> +   bool lower_vote_eq;

Since there are potentially multiple ways to lower these (voteAny(x)
-> !voteAll(!x), using ballotARB(), etc.), and the way they're lowered
is a little... unexpected (although admittedly legal!), why don't we
use a more descriptive name, like lower_vote_*_trivial? While we're at
it, I highly doubt that an implementation would want this kind of
lowering for just one of the intrinsics, so we can merge this into a
single flag, say lower_vote_trivial.

> +
> /**
>  * Does the driver support real 32-bit integers?  (Otherwise, integers
>  * are simulated by floats.)
> diff --git a/src/compiler/nir/nir_opt_intrinsics.c 
> b/src/compiler/nir/nir_opt_intrinsics.c
> index b63449b4fe..0cd75d8b28 100644
> --- a/src/compiler/nir/nir_opt_intrinsics.c
> +++ b/src/compiler/nir/nir_opt_intrinsics.c
> @@ -47,7 +47,7 @@ opt_intrinsics_impl(nir_function_impl *impl)
>   switch (intrin->intrinsic) {
>   case nir_intrinsic_vote_any: {
>  nir_const_value *val = nir_src_as_const_value(intrin->src[0]);
> -if (!val)
> +if (!val || b.shader->options->lower_vote_any)
> continue;
>
>  replacement = nir_imm_int(, val->i32[0]);
> @@ -55,7 +55,7 @@ opt_intrinsics_impl(nir_function_impl *impl)
>   }
>   case nir_intrinsic_vote_all: {
>  nir_const_value *val = nir_src_as_const_value(intrin->src[0]);
> -if (!val)
> +if (!val || b.shader->options->lower_vote_all)
> continue;
>
>  replacement = nir_imm_int(, val->i32[0]);
> @@ -63,7 +63,7 @@ opt_intrinsics_impl(nir_function_impl *impl)
>   }
>   case nir_intrinsic_vote_eq: {
>  nir_const_value *val = nir_src_as_const_value(intrin->src[0]);
> -if (!val)
> +if (!val || b.shader->options->lower_vote_eq)
> continue;
>
>  replacement = nir_imm_int(, NIR_TRUE);
> --
> 2.13.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

[Mesa-dev] [PATCH 03/20] nir: Support lowering vote intrinsics

[Matt Turner]

... trivially (as allowed by the spec!) by reusing the existing
nir_opt_intrinsics code.
---
 src/compiler/nir/nir.h| 4 
 src/compiler/nir/nir_opt_intrinsics.c | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index 44a1d0887e..401c41f155 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -1821,6 +1821,10 @@ typedef struct nir_shader_compiler_options {
bool lower_extract_byte;
bool lower_extract_word;
 
+   bool lower_vote_any;
+   bool lower_vote_all;
+   bool lower_vote_eq;
+
/**
 * Does the driver support real 32-bit integers?  (Otherwise, integers
 * are simulated by floats.)
diff --git a/src/compiler/nir/nir_opt_intrinsics.c 
b/src/compiler/nir/nir_opt_intrinsics.c
index b63449b4fe..0cd75d8b28 100644
--- a/src/compiler/nir/nir_opt_intrinsics.c
+++ b/src/compiler/nir/nir_opt_intrinsics.c
@@ -47,7 +47,7 @@ opt_intrinsics_impl(nir_function_impl *impl)
  switch (intrin->intrinsic) {
  case nir_intrinsic_vote_any: {
 nir_const_value *val = nir_src_as_const_value(intrin->src[0]);
-if (!val)
+if (!val || b.shader->options->lower_vote_any)
continue;
 
 replacement = nir_imm_int(, val->i32[0]);
@@ -55,7 +55,7 @@ opt_intrinsics_impl(nir_function_impl *impl)
  }
  case nir_intrinsic_vote_all: {
 nir_const_value *val = nir_src_as_const_value(intrin->src[0]);
-if (!val)
+if (!val || b.shader->options->lower_vote_all)
continue;
 
 replacement = nir_imm_int(, val->i32[0]);
@@ -63,7 +63,7 @@ opt_intrinsics_impl(nir_function_impl *impl)
  }
  case nir_intrinsic_vote_eq: {
 nir_const_value *val = nir_src_as_const_value(intrin->src[0]);
-if (!val)
+if (!val || b.shader->options->lower_vote_eq)
continue;
 
 replacement = nir_imm_int(, NIR_TRUE);
-- 
2.13.0

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