[Mesa-dev] [PATCH 13/21] intel/cs: Grow prog_data::param on-demand for thread_local_id_index

[Jordan Justen]

On 2017-09-29 14:25:13, Jason Ekstrand wrote:
> Instead of making the caller of brw_compile_cs add something to the
> param array for thread_local_id_index, just add it on-demand in
> brw_nir_intrinsics and grow the array.  This is now safe to do because
> everyone is now using ralloc for prog_data::param.
> ---
>  src/intel/compiler/brw_fs.cpp           |  8 --------
>  src/intel/compiler/brw_nir_intrinsics.c | 16 +++++++++-------
>  src/intel/vulkan/anv_pipeline.c         |  4 ----
>  src/mesa/drivers/dri/i965/brw_cs.c      |  3 ---
>  4 files changed, 9 insertions(+), 22 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
> index 4c1e614..52b6628 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -6753,14 +6753,6 @@ brw_compile_cs(const struct brw_compiler *compiler, void *log_data,
>     nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
>     shader = brw_nir_apply_sampler_key(shader, compiler, &key->tex, true);
>  
> -   /* Now that we cloned the nir_shader, we can update num_uniforms based on
> -    * the thread_local_id_index.
> -    */
> -   assert(prog_data->thread_local_id_index >= 0);
> -   shader->num_uniforms =
> -      MAX2(shader->num_uniforms,
> -           (unsigned)4 * (prog_data->thread_local_id_index + 1));
> -
>     brw_nir_lower_intrinsics(shader, &prog_data->base);
>     shader = brw_postprocess_nir(shader, compiler, true);
>  
> diff --git a/src/intel/compiler/brw_nir_intrinsics.c b/src/intel/compiler/brw_nir_intrinsics.c
> index abbbc6f..83c6c0e 100644
> --- a/src/intel/compiler/brw_nir_intrinsics.c
> +++ b/src/intel/compiler/brw_nir_intrinsics.c
> @@ -33,12 +33,12 @@ struct lower_intrinsics_state {
>     nir_function_impl *impl;
>     bool progress;
>     nir_builder builder;
> -   bool cs_thread_id_used;
>  };
>  
>  static nir_ssa_def *
>  read_thread_local_id(struct lower_intrinsics_state *state)
>  {
> +   struct brw_stage_prog_data *prog_data = state->prog_data;
>     nir_builder *b = &state->builder;
>     nir_shader *nir = state->nir;
>     const unsigned *sizes = nir->info.cs.local_size;
> @@ -50,9 +50,12 @@ read_thread_local_id(struct lower_intrinsics_state *state)
>     if (group_size <= 8)
>        return nir_imm_int(b, 0);
>  
> -   assert(state->cs_prog_data->thread_local_id_index >= 0);
> -   state->cs_thread_id_used = true;
> -   const int id_index = state->cs_prog_data->thread_local_id_index;
> +   if (state->cs_prog_data->thread_local_id_index == -1) {
> +      state->cs_prog_data->thread_local_id_index = prog_data->nr_params;
> +      brw_stage_prog_data_add_params(prog_data, 1);
> +      nir->num_uniforms += 4;
> +   }
> +   unsigned id_index = state->cs_prog_data->thread_local_id_index;
>  
>     nir_intrinsic_instr *load =
>        nir_intrinsic_instr_create(nir, nir_intrinsic_load_uniform);
> @@ -168,6 +171,8 @@ brw_nir_lower_intrinsics(nir_shader *nir, struct brw_stage_prog_data *prog_data)
>     state.nir = nir;
>     state.prog_data = prog_data;
>  
> +   state.cs_prog_data->thread_local_id_index = -1;

I intended brw_nir_lower_intrinsics to be potentially usable for all
stages. It looks like it is only ever called for compute. Maybe it
should be renamed brw_nir_lower_cs_intrinsics if we are going to make
it assume it is only used for compute.

My recommendation here would be:

   if (nir->stage == MESA_SHADER_COMPUTE)
      state.cs_prog_data->thread_local_id_index = -1;

I'm also fine with making it compute specific if that's what you
prefer.

-Jordan

> +
>     do {
>        state.progress = false;
>        nir_foreach_function(function, nir) {
> @@ -179,8 +184,5 @@ brw_nir_lower_intrinsics(nir_shader *nir, struct brw_stage_prog_data *prog_data)
>        progress |= state.progress;
>     } while (state.progress);
>  
> -   if (nir->stage == MESA_SHADER_COMPUTE && !state.cs_thread_id_used)
> -      state.cs_prog_data->thread_local_id_index = -1;
> -
>     return progress;
>  }
> diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c
> index 5ebe5eb..191ae55 100644
> --- a/src/intel/vulkan/anv_pipeline.c
> +++ b/src/intel/vulkan/anv_pipeline.c
> @@ -407,10 +407,6 @@ anv_pipeline_compile(struct anv_pipeline *pipeline,
>        pipeline->needs_data_cache = true;
>     }
>  
> -   if (stage == MESA_SHADER_COMPUTE)
> -      ((struct brw_cs_prog_data *)prog_data)->thread_local_id_index =
> -         prog_data->nr_params++; /* The CS Thread ID uniform */
> -
>     if (nir->info.num_ssbos > 0)
>        pipeline->needs_data_cache = true;
>  
> diff --git a/src/mesa/drivers/dri/i965/brw_cs.c b/src/mesa/drivers/dri/i965/brw_cs.c
> index 41a5431..dacb25e 100644
> --- a/src/mesa/drivers/dri/i965/brw_cs.c
> +++ b/src/mesa/drivers/dri/i965/brw_cs.c
> @@ -83,9 +83,6 @@ brw_codegen_cs_prog(struct brw_context *brw,
>      */
>     int param_count = cp->program.nir->num_uniforms / 4;
>  
> -   /* The backend also sometimes add a param for the thread local id. */
> -   prog_data.thread_local_id_index = param_count++;
> -
>     prog_data.base.param = rzalloc_array(NULL, uint32_t, param_count);
>     prog_data.base.pull_param = rzalloc_array(NULL, uint32_t, param_count);
>     prog_data.base.nr_params = param_count;
> -- 
> 2.5.0.400.gff86faf
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev