[Mesa-dev] [PATCH 06/20] i965/fs: Implement ARB_shader_group_vote operations

[Matt Turner]

On Mon, Jul 17, 2017 at 4:44 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Thursday, July 6, 2017 4:48:16 PM PDT Matt Turner wrote:
>> ---
>>  src/intel/compiler/brw_fs_nir.cpp | 36 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/src/intel/compiler/brw_fs_nir.cpp b/src/intel/compiler/brw_fs_nir.cpp
>> index a9dce42c38..264398f38e 100644
>> --- a/src/intel/compiler/brw_fs_nir.cpp
>> +++ b/src/intel/compiler/brw_fs_nir.cpp
>> @@ -4090,6 +4090,42 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
>>        break;
>>     }
>>
>> +   case nir_intrinsic_vote_any: {
>> +      const fs_builder ubld = bld.exec_all();
>> +      ubld.MOV(brw_flag_reg(0, 0), brw_imm_uw(0));
>> +      bld.CMP(bld.null_reg_d(), get_nir_src(instr->src[0]), brw_imm_d(0), BRW_CONDITIONAL_NZ);
>> +      bld.MOV(dest, brw_imm_d(-1));
>
> This might be easier to read as
>
>       bld.MOV(dest, brw_imm_ud(~0));
>
> at least, I normally think of true as ~0, not -1...though they're
> obviously the same.  Totally up to you, though.
>
>> +      set_predicate(dispatch_width == 8 ?
>> +                    BRW_PREDICATE_ALIGN1_ANY8H :
>> +                    BRW_PREDICATE_ALIGN1_ANY16H,
>> +                    bld.SEL(dest, dest, brw_imm_d(0)));
>> +      break;
>> +   }
>> +   case nir_intrinsic_vote_all: {
>> +      const fs_builder ubld = bld.exec_all();
>> +      ubld.MOV(brw_flag_reg(0, 0), brw_imm_uw(0xffff));
>> +      bld.CMP(bld.null_reg_d(), get_nir_src(instr->src[0]), brw_imm_d(0), BRW_CONDITIONAL_NZ);
>
> At a glance, it's not obvious why you're explicitly initializing the flag
> register value with a MOV, rather than just doing the CMP.  I believe this
> is because inactive channels still affect the ANY/ALL predicates, and you
> want to ignore those here.  Maybe add a comment to that effect?

Definitely a good thing to document. I've added the following comment
immediately above the flag-initializing MOV in all three cases:

      /* The any/all predicates do not consider channel enables. To prevent
       * dead channels from affecting the result, we initialize the flag with
       * with the identity value for the logical operation.
       */