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

[Kenneth Graunke]

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?

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +      bld.MOV(dest, brw_imm_d(-1));
> +      set_predicate(dispatch_width == 8 ?
> +                    BRW_PREDICATE_ALIGN1_ALL8H :
> +                    BRW_PREDICATE_ALIGN1_ALL16H,
> +                    bld.SEL(dest, dest, brw_imm_d(0)));
> +      break;
> +   }
> +   case nir_intrinsic_vote_eq: {
> +      fs_reg value = get_nir_src(instr->src[0]);
> +      fs_reg uniformized = bld.emit_uniformize(value);
> +
> +      const fs_builder ubld = bld.exec_all();
> +      ubld.MOV(brw_flag_reg(0, 0), brw_imm_uw(0xffff));
> +      bld.CMP(bld.null_reg_d(), value, uniformized, BRW_CONDITIONAL_Z);
> +      bld.MOV(dest, brw_imm_d(-1));
> +      set_predicate(dispatch_width == 8 ?
> +                    BRW_PREDICATE_ALIGN1_ALL8H :
> +                    BRW_PREDICATE_ALIGN1_ALL16H,
> +                    bld.SEL(dest, dest, brw_imm_d(0)));
> +      break;
> +   }
>     default:
>        unreachable("unknown intrinsic");
>     }
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170717/c1afdf5d/attachment-0001.sig>