[Mesa-dev] [PATCH] nir: Use a system value for gl_PrimitiveIDIn.

[Iago Toral]

On Mon, 2015-09-28 at 23:05 -0700, Kenneth Graunke wrote:
> At least on Intel hardware, gl_PrimitiveIDIn comes in as a special part
> of the payload rather than a normal input.  This is typically what we
> use system values for.  Dave and Ilia also agree that a system value
> would be nicer.
> 
> At some point, we should change it at the GLSL IR level as well.  But
> that requires changing most of the drivers.  For now, let's at least
> make NIR do the right thing, which is easy.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/glsl/nir/glsl_to_nir.cpp                  | 5 +++++
>  src/glsl/nir/nir.c                            | 5 ++++-
>  src/glsl/nir/nir_intrinsics.h                 | 1 +
>  src/glsl/shader_enums.h                       | 2 +-
>  src/mesa/drivers/dri/i965/brw_vec4_gs_nir.cpp | 9 +++++++++
>  5 files changed, 20 insertions(+), 2 deletions(-)
> 
> I bypassed most of the system value boilerplate in the backend.  Notably,
> this means we just access g1 directly rather than moving it to a VGRF at
> the start of the program and using that later.  This means more HW_REG
> usage, but it also means less seemingly pointless copies.
> 
> I'm hoping to simplify the handling of other system values too, but I'm
> waiting until we delete the GLSL IR code paths first.
> 
> diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
> index f03a107..c0b2634 100644
> --- a/src/glsl/nir/glsl_to_nir.cpp
> +++ b/src/glsl/nir/glsl_to_nir.cpp
> @@ -271,6 +271,11 @@ nir_visitor::visit(ir_variable *ir)
>           /* For whatever reason, GLSL IR makes gl_FrontFacing an input */
>           var->data.location = SYSTEM_VALUE_FRONT_FACE;
>           var->data.mode = nir_var_system_value;
> +      } else if (shader->stage == MESA_SHADER_GEOMETRY &&
> +                 ir->data.location == VARYING_SLOT_PRIMITIVE_ID) {
> +         /* For whatever reason, GLSL IR makes gl_PrimitiveIDIn an input */
> +         var->data.location = SYSTEM_VALUE_PRIMITIVE_ID;
> +         var->data.mode = nir_var_system_value;
>        } else {
>           var->data.mode = nir_var_shader_in;
>        }
> diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c
> index 1206bb4..7f30b8a 100644
> --- a/src/glsl/nir/nir.c
> +++ b/src/glsl/nir/nir.c
> @@ -1487,10 +1487,11 @@ nir_intrinsic_from_system_value(gl_system_value val)
>        return nir_intrinsic_load_local_invocation_id;
>     case SYSTEM_VALUE_WORK_GROUP_ID:
>        return nir_intrinsic_load_work_group_id;
> +   case SYSTEM_VALUE_PRIMITIVE_ID:
> +      return nir_intrinsic_load_primitive_id;
>     /* FINISHME: Add tessellation intrinsics.
>     case SYSTEM_VALUE_TESS_COORD:
>     case SYSTEM_VALUE_VERTICES_IN:
> -   case SYSTEM_VALUE_PRIMITIVE_ID:
>     case SYSTEM_VALUE_TESS_LEVEL_OUTER:
>     case SYSTEM_VALUE_TESS_LEVEL_INNER:
>      */
> @@ -1525,6 +1526,8 @@ nir_system_value_from_intrinsic(nir_intrinsic_op intrin)
>        return SYSTEM_VALUE_LOCAL_INVOCATION_ID;
>     case nir_intrinsic_load_work_group_id:
>        return SYSTEM_VALUE_WORK_GROUP_ID;
> +   case nir_intrinsic_load_primitive_id:
> +      return SYSTEM_VALUE_PRIMITIVE_ID;
>     /* FINISHME: Add tessellation intrinsics.
>        return SYSTEM_VALUE_TESS_COORD;
>        return SYSTEM_VALUE_VERTICES_IN;
> diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
> index 06f1b02..0d93b12 100644
> --- a/src/glsl/nir/nir_intrinsics.h
> +++ b/src/glsl/nir/nir_intrinsics.h
> @@ -194,6 +194,7 @@ SYSTEM_VALUE(instance_id, 1, 0)
>  SYSTEM_VALUE(sample_id, 1, 0)
>  SYSTEM_VALUE(sample_pos, 2, 0)
>  SYSTEM_VALUE(sample_mask_in, 1, 0)
> +SYSTEM_VALUE(primitive_id, 1, 0)
>  SYSTEM_VALUE(invocation_id, 1, 0)
>  SYSTEM_VALUE(local_invocation_id, 3, 0)
>  SYSTEM_VALUE(work_group_id, 3, 0)
> diff --git a/src/glsl/shader_enums.h b/src/glsl/shader_enums.h
> index 3978007..f6b71a3 100644
> --- a/src/glsl/shader_enums.h
> +++ b/src/glsl/shader_enums.h
> @@ -399,7 +399,7 @@ typedef enum
>     /*@{*/
>     SYSTEM_VALUE_TESS_COORD,
>     SYSTEM_VALUE_VERTICES_IN,    /**< Tessellation vertices in input patch */
> -   SYSTEM_VALUE_PRIMITIVE_ID,   /**< (currently not used by GS) */
> +   SYSTEM_VALUE_PRIMITIVE_ID,
>     SYSTEM_VALUE_TESS_LEVEL_OUTER, /**< TES input */
>     SYSTEM_VALUE_TESS_LEVEL_INNER, /**< TES input */
>     /*@}*/
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_gs_nir.cpp
> index 4f4e1e1..26766a0 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_gs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs_nir.cpp
> @@ -72,6 +72,9 @@ vec4_gs_visitor::nir_setup_system_value_intrinsic(nir_intrinsic_instr *instr)
>     dst_reg *reg;
>  
>     switch (instr->intrinsic) {
> +   case nir_intrinsic_load_primitive_id:

Maybe add a one-line comment saying that we will access grf1 directly so
there is no need to setup a vgrf here? Either way:

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

> +      break;
> +
>     case nir_intrinsic_load_invocation_id:
>        reg = &this->nir_system_values[SYSTEM_VALUE_INVOCATION_ID];
>        if (reg->file == BAD_FILE)
> @@ -111,6 +114,12 @@ vec4_gs_visitor::nir_emit_intrinsic(nir_intrinsic_instr *instr)
>           retype(get_nir_src(instr->src[0], 1), BRW_REGISTER_TYPE_UD);
>        break;
>  
> +   case nir_intrinsic_load_primitive_id:
> +      assert(c->prog_data.include_primitive_id);
> +      dest = get_nir_dest(instr->dest, BRW_REGISTER_TYPE_D);
> +      emit(MOV(dest, retype(brw_vec4_grf(1, 0), BRW_REGISTER_TYPE_D)));
> +      break;
> +
>     case nir_intrinsic_load_invocation_id: {
>        src_reg invocation_id =
>           src_reg(nir_system_values[SYSTEM_VALUE_INVOCATION_ID]);