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

[Matt Turner]

On Tue, Jul 18, 2017 at 1:34 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> On Mon, Jul 10, 2017 at 10:18 AM, Matt Turner <mattst88 at gmail.com> wrote:
>> On Thu, Jul 6, 2017 at 8:04 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>> On Thu, Jul 6, 2017 at 4:48 PM, Matt Turner <mattst88 at gmail.com> 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(&b, 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.