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

[Connor Abbott]

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;

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);

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.