[Mesa-dev] [PATCH 17/22] nir: rename global to private memory

Jason Ekstrand jason at jlekstrand.net
Wed Nov 14 20:27:57 UTC 2018


On Tue, Nov 13, 2018 at 9:49 AM Karol Herbst <kherbst at redhat.com> wrote:

> the naming is a bit confusing no matter how you look at it. Within OpenCL
> "global" memory is memory accessible from all threads. glsl "global" memory
> normally refers to shader thread private memory declared at global scope.
> As
> we already use "shared" for memory shared across all thrads of a work group
> the solution where everybody could be happy with is to rename "global" to
> "private" and use "global" later for memory usually stored within system
> accessible memory (be it VRAM or system RAM if keeping SVM in mind).
>

Yeah.... There's just no good way to pick these names.  It's been kicking
around in my brain for a while and I still don't have anything I like.  One
thing I will say, though, is that if we're going to rename global to
private, I think I'd like to rename local to function at the same time.
That way, it's clear that they map to the SPIR-V thing and we're no longer
using the "global/local" terminology from C.  If we do that, then I think
I'm fine with this change.

--Jason


> Signed-off-by: Karol Herbst <kherbst at redhat.com>
> ---
>  src/compiler/glsl/glsl_to_nir.cpp                |  4 ++--
>  src/compiler/nir/nir.c                           |  2 +-
>  src/compiler/nir/nir.h                           |  2 +-
>  src/compiler/nir/nir_linking_helpers.c           |  2 +-
>  .../nir/nir_lower_constant_initializers.c        |  2 +-
>  .../nir/nir_lower_global_vars_to_local.c         |  4 ++--
>  src/compiler/nir/nir_lower_io_to_temporaries.c   |  2 +-
>  src/compiler/nir/nir_opt_copy_prop_vars.c        |  4 ++--
>  src/compiler/nir/nir_opt_dead_write_vars.c       |  2 +-
>  src/compiler/nir/nir_print.c                     |  4 ++--
>  src/compiler/nir/nir_remove_dead_variables.c     |  4 ++--
>  src/compiler/nir/nir_split_vars.c                | 16 ++++++++--------
>  src/compiler/nir/tests/vars_tests.cpp            |  2 +-
>  src/compiler/spirv/vtn_private.h                 |  2 +-
>  src/compiler/spirv/vtn_variables.c               |  6 +++---
>  src/gallium/auxiliary/nir/tgsi_to_nir.c          |  2 +-
>  src/mesa/state_tracker/st_glsl_to_nir.cpp        |  2 +-
>  17 files changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/src/compiler/glsl/glsl_to_nir.cpp
> b/src/compiler/glsl/glsl_to_nir.cpp
> index 0479f8fcfe4..8564cd89b5a 100644
> --- a/src/compiler/glsl/glsl_to_nir.cpp
> +++ b/src/compiler/glsl/glsl_to_nir.cpp
> @@ -312,7 +312,7 @@ nir_visitor::visit(ir_variable *ir)
>     case ir_var_auto:
>     case ir_var_temporary:
>        if (is_global)
> -         var->data.mode = nir_var_global;
> +         var->data.mode = nir_var_private;
>        else
>           var->data.mode = nir_var_local;
>        break;
> @@ -1433,7 +1433,7 @@ nir_visitor::visit(ir_expression *ir)
>            * sense, we'll just turn it into a load which will probably
>            * eventually end up as an SSA definition.
>            */
> -         assert(this->deref->mode == nir_var_global);
> +         assert(this->deref->mode == nir_var_private);
>           op = nir_intrinsic_load_deref;
>        }
>
> diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c
> index 249b9357c3f..27f5d1b7bca 100644
> --- a/src/compiler/nir/nir.c
> +++ b/src/compiler/nir/nir.c
> @@ -129,7 +129,7 @@ nir_shader_add_variable(nir_shader *shader,
> nir_variable *var)
>        assert(!"nir_shader_add_variable cannot be used for local
> variables");
>        break;
>
> -   case nir_var_global:
> +   case nir_var_private:
>        exec_list_push_tail(&shader->globals, &var->node);
>        break;
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index 89c28e36618..78f3204d3e2 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -96,7 +96,7 @@ typedef struct {
>  typedef enum {
>     nir_var_shader_in       = (1 << 0),
>     nir_var_shader_out      = (1 << 1),
> -   nir_var_global          = (1 << 2),
> +   nir_var_private         = (1 << 2),
>     nir_var_local           = (1 << 3),
>     nir_var_uniform         = (1 << 4),
>     nir_var_shader_storage  = (1 << 5),
> diff --git a/src/compiler/nir/nir_linking_helpers.c
> b/src/compiler/nir/nir_linking_helpers.c
> index a05890ada43..d8358e08e5a 100644
> --- a/src/compiler/nir/nir_linking_helpers.c
> +++ b/src/compiler/nir/nir_linking_helpers.c
> @@ -134,7 +134,7 @@ nir_remove_unused_io_vars(nir_shader *shader, struct
> exec_list *var_list,
>        if (!(other_stage & get_variable_io_mask(var, shader->info.stage)))
> {
>           /* This one is invalid, make it a global variable instead */
>           var->data.location = 0;
> -         var->data.mode = nir_var_global;
> +         var->data.mode = nir_var_private;
>
>           exec_node_remove(&var->node);
>           exec_list_push_tail(&shader->globals, &var->node);
> diff --git a/src/compiler/nir/nir_lower_constant_initializers.c
> b/src/compiler/nir/nir_lower_constant_initializers.c
> index 4e9cea46157..932a32b3c9c 100644
> --- a/src/compiler/nir/nir_lower_constant_initializers.c
> +++ b/src/compiler/nir/nir_lower_constant_initializers.c
> @@ -98,7 +98,7 @@ nir_lower_constant_initializers(nir_shader *shader,
> nir_variable_mode modes)
>     if (modes & nir_var_shader_out)
>        progress |= lower_const_initializer(&builder, &shader->outputs);
>
> -   if (modes & nir_var_global)
> +   if (modes & nir_var_private)
>        progress |= lower_const_initializer(&builder, &shader->globals);
>
>     if (modes & nir_var_system_value)
> diff --git a/src/compiler/nir/nir_lower_global_vars_to_local.c
> b/src/compiler/nir/nir_lower_global_vars_to_local.c
> index be99cf9ad02..6c6d9a9d25c 100644
> --- a/src/compiler/nir/nir_lower_global_vars_to_local.c
> +++ b/src/compiler/nir/nir_lower_global_vars_to_local.c
> @@ -36,7 +36,7 @@ static void
>  register_var_use(nir_variable *var, nir_function_impl *impl,
>                   struct hash_table *var_func_table)
>  {
> -   if (var->data.mode != nir_var_global)
> +   if (var->data.mode != nir_var_private)
>        return;
>
>     struct hash_entry *entry =
> @@ -89,7 +89,7 @@ nir_lower_global_vars_to_local(nir_shader *shader)
>        nir_variable *var = (void *)entry->key;
>        nir_function_impl *impl = entry->data;
>
> -      assert(var->data.mode == nir_var_global);
> +      assert(var->data.mode == nir_var_private);
>
>        if (impl != NULL) {
>           exec_node_remove(&var->node);
> diff --git a/src/compiler/nir/nir_lower_io_to_temporaries.c
> b/src/compiler/nir/nir_lower_io_to_temporaries.c
> index b83aaf46e6a..2487add33ed 100644
> --- a/src/compiler/nir/nir_lower_io_to_temporaries.c
> +++ b/src/compiler/nir/nir_lower_io_to_temporaries.c
> @@ -134,7 +134,7 @@ create_shadow_temp(struct lower_io_state *state,
> nir_variable *var)
>     /* Give the original a new name with @<mode>-temp appended */
>     const char *mode = (temp->data.mode == nir_var_shader_in) ? "in" :
> "out";
>     temp->name = ralloc_asprintf(var, "%s@%s-temp", mode, nvar->name);
> -   temp->data.mode = nir_var_global;
> +   temp->data.mode = nir_var_private;
>     temp->data.read_only = false;
>     temp->data.fb_fetch_output = false;
>     temp->data.compact = false;
> diff --git a/src/compiler/nir/nir_opt_copy_prop_vars.c
> b/src/compiler/nir/nir_opt_copy_prop_vars.c
> index 48070cd5e31..95f0bbd5b77 100644
> --- a/src/compiler/nir/nir_opt_copy_prop_vars.c
> +++ b/src/compiler/nir/nir_opt_copy_prop_vars.c
> @@ -134,7 +134,7 @@ gather_vars_written(struct copy_prop_var_state *state,
>        nir_foreach_instr(instr, block) {
>           if (instr->type == nir_instr_type_call) {
>              written->modes |= nir_var_shader_out |
> -                              nir_var_global |
> +                              nir_var_private |
>                                nir_var_local |
>                                nir_var_shader_storage |
>                                nir_var_shared;
> @@ -603,7 +603,7 @@ copy_prop_vars_block(struct copy_prop_var_state *state,
>     nir_foreach_instr_safe(instr, block) {
>        if (instr->type == nir_instr_type_call) {
>           apply_barrier_for_modes(copies, nir_var_shader_out |
> -                                         nir_var_global |
> +                                         nir_var_private |
>                                           nir_var_local |
>                                           nir_var_shader_storage |
>                                           nir_var_shared);
> diff --git a/src/compiler/nir/nir_opt_dead_write_vars.c
> b/src/compiler/nir/nir_opt_dead_write_vars.c
> index dd949998cc8..0fbbe63f436 100644
> --- a/src/compiler/nir/nir_opt_dead_write_vars.c
> +++ b/src/compiler/nir/nir_opt_dead_write_vars.c
> @@ -120,7 +120,7 @@ remove_dead_write_vars_local(void *mem_ctx, nir_block
> *block)
>     nir_foreach_instr_safe(instr, block) {
>        if (instr->type == nir_instr_type_call) {
>           clear_unused_for_modes(&unused_writes, nir_var_shader_out |
> -                                                nir_var_global |
> +                                                nir_var_private |
>                                                  nir_var_local |
>                                                  nir_var_shader_storage |
>                                                  nir_var_shared);
> diff --git a/src/compiler/nir/nir_print.c b/src/compiler/nir/nir_print.c
> index 0de82800f8c..88f91087134 100644
> --- a/src/compiler/nir/nir_print.c
> +++ b/src/compiler/nir/nir_print.c
> @@ -416,8 +416,8 @@ get_variable_mode_str(nir_variable_mode mode, bool
> want_local_global_mode)
>        return "system";
>     case nir_var_shared:
>        return "shared";
> -   case nir_var_global:
> -      return want_local_global_mode ? "global" : "";
> +   case nir_var_private:
> +      return want_local_global_mode ? "private" : "";
>     case nir_var_local:
>        return want_local_global_mode ? "local" : "";
>     default:
> diff --git a/src/compiler/nir/nir_remove_dead_variables.c
> b/src/compiler/nir/nir_remove_dead_variables.c
> index fadc51a6977..d09e823c517 100644
> --- a/src/compiler/nir/nir_remove_dead_variables.c
> +++ b/src/compiler/nir/nir_remove_dead_variables.c
> @@ -71,7 +71,7 @@ add_var_use_deref(nir_deref_instr *deref, struct set
> *live)
>      * all means we need to keep it alive.
>      */
>     assert(deref->mode == deref->var->data.mode);
> -   if (!(deref->mode & (nir_var_local | nir_var_global | nir_var_shared))
> ||
> +   if (!(deref->mode & (nir_var_local | nir_var_private |
> nir_var_shared)) ||
>         deref_used_for_not_store(deref))
>        _mesa_set_add(live, deref->var);
>  }
> @@ -175,7 +175,7 @@ nir_remove_dead_variables(nir_shader *shader,
> nir_variable_mode modes)
>     if (modes & nir_var_shader_out)
>        progress = remove_dead_vars(&shader->outputs, live) || progress;
>
> -   if (modes & nir_var_global)
> +   if (modes & nir_var_private)
>        progress = remove_dead_vars(&shader->globals, live) || progress;
>
>     if (modes & nir_var_system_value)
> diff --git a/src/compiler/nir/nir_split_vars.c
> b/src/compiler/nir/nir_split_vars.c
> index bf9205c5150..9d1a096268c 100644
> --- a/src/compiler/nir/nir_split_vars.c
> +++ b/src/compiler/nir/nir_split_vars.c
> @@ -259,10 +259,10 @@ nir_split_struct_vars(nir_shader *shader,
> nir_variable_mode modes)
>        _mesa_hash_table_create(mem_ctx, _mesa_hash_pointer,
>                                _mesa_key_pointer_equal);
>
> -   assert((modes & (nir_var_global | nir_var_local)) == modes);
> +   assert((modes & (nir_var_private | nir_var_local)) == modes);
>
>     bool has_global_splits = false;
> -   if (modes & nir_var_global) {
> +   if (modes & nir_var_private) {
>        has_global_splits = split_var_list_structs(shader, NULL,
>                                                   &shader->globals,
>                                                   var_field_map, mem_ctx);
> @@ -795,10 +795,10 @@ nir_split_array_vars(nir_shader *shader,
> nir_variable_mode modes)
>        _mesa_hash_table_create(mem_ctx, _mesa_hash_pointer,
>                                _mesa_key_pointer_equal);
>
> -   assert((modes & (nir_var_global | nir_var_local)) == modes);
> +   assert((modes & (nir_var_private | nir_var_local)) == modes);
>
>     bool has_global_array = false;
> -   if (modes & nir_var_global) {
> +   if (modes & nir_var_private) {
>        has_global_array = init_var_list_array_infos(&shader->globals,
>                                                     var_info_map, mem_ctx);
>     }
> @@ -827,7 +827,7 @@ nir_split_array_vars(nir_shader *shader,
> nir_variable_mode modes)
>     }
>
>     bool has_global_splits = false;
> -   if (modes & nir_var_global) {
> +   if (modes & nir_var_private) {
>        has_global_splits = split_var_list_arrays(shader, NULL,
>                                                  &shader->globals,
>                                                  var_info_map, mem_ctx);
> @@ -1494,7 +1494,7 @@ function_impl_has_vars_with_modes(nir_function_impl
> *impl,
>  {
>     nir_shader *shader = impl->function->shader;
>
> -   if ((modes & nir_var_global) && !exec_list_is_empty(&shader->globals))
> +   if ((modes & nir_var_private) && !exec_list_is_empty(&shader->globals))
>        return true;
>
>     if ((modes & nir_var_local) && !exec_list_is_empty(&impl->locals))
> @@ -1515,7 +1515,7 @@ function_impl_has_vars_with_modes(nir_function_impl
> *impl,
>  bool
>  nir_shrink_vec_array_vars(nir_shader *shader, nir_variable_mode modes)
>  {
> -   assert((modes & (nir_var_global | nir_var_local)) == modes);
> +   assert((modes & (nir_var_private | nir_var_local)) == modes);
>
>     void *mem_ctx = ralloc_context(NULL);
>
> @@ -1544,7 +1544,7 @@ nir_shrink_vec_array_vars(nir_shader *shader,
> nir_variable_mode modes)
>     }
>
>     bool globals_shrunk = false;
> -   if (modes & nir_var_global)
> +   if (modes & nir_var_private)
>        globals_shrunk = shrink_vec_var_list(&shader->globals,
> var_usage_map);
>
>     bool progress = false;
> diff --git a/src/compiler/nir/tests/vars_tests.cpp
> b/src/compiler/nir/tests/vars_tests.cpp
> index 32763d2db64..666d22643dd 100644
> --- a/src/compiler/nir/tests/vars_tests.cpp
> +++ b/src/compiler/nir/tests/vars_tests.cpp
> @@ -191,7 +191,7 @@ TEST_F(nir_redundant_load_vars_test,
> invalidate_inside_if_block)
>      * if statement.  They should be invalidated accordingly.
>      */
>
> -   nir_variable **g = create_many_int(nir_var_global, "g", 3);
> +   nir_variable **g = create_many_int(nir_var_private, "g", 3);
>     nir_variable **out = create_many_int(nir_var_shader_out, "out", 3);
>
>     nir_load_var(b, g[0]);
> diff --git a/src/compiler/spirv/vtn_private.h
> b/src/compiler/spirv/vtn_private.h
> index d7d5b8db198..86f98083f58 100644
> --- a/src/compiler/spirv/vtn_private.h
> +++ b/src/compiler/spirv/vtn_private.h
> @@ -418,7 +418,7 @@ struct vtn_access_chain {
>
>  enum vtn_variable_mode {
>     vtn_variable_mode_local,
> -   vtn_variable_mode_global,
> +   vtn_variable_mode_private,
>     vtn_variable_mode_uniform,
>     vtn_variable_mode_ubo,
>     vtn_variable_mode_ssbo,
> diff --git a/src/compiler/spirv/vtn_variables.c
> b/src/compiler/spirv/vtn_variables.c
> index e7654b768af..5738941ffb6 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1556,8 +1556,8 @@ vtn_storage_class_to_mode(struct vtn_builder *b,
>        nir_mode = nir_var_shader_out;
>        break;
>     case SpvStorageClassPrivate:
> -      mode = vtn_variable_mode_global;
> -      nir_mode = nir_var_global;
> +      mode = vtn_variable_mode_private;
> +      nir_mode = nir_var_private;
>        break;
>     case SpvStorageClassFunction:
>        mode = vtn_variable_mode_local;
> @@ -1721,7 +1721,7 @@ vtn_create_variable(struct vtn_builder *b, struct
> vtn_value *val,
>
>     switch (var->mode) {
>     case vtn_variable_mode_local:
> -   case vtn_variable_mode_global:
> +   case vtn_variable_mode_private:
>     case vtn_variable_mode_uniform:
>        /* For these, we create the variable normally */
>        var->var = rzalloc(b->shader, nir_variable);
> diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
> b/src/gallium/auxiliary/nir/tgsi_to_nir.c
> index b108c05d895..c95c45279f3 100644
> --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
> +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
> @@ -182,7 +182,7 @@ ttn_emit_declaration(struct ttn_compile *c)
>           nir_variable *var = rzalloc(b->shader, nir_variable);
>
>           var->type = glsl_array_type(glsl_vec4_type(), array_size);
> -         var->data.mode = nir_var_global;
> +         var->data.mode = nir_var_private;
>           var->name = ralloc_asprintf(var, "arr_%d", decl->Array.ArrayID);
>
>           exec_list_push_tail(&b->shader->globals, &var->node);
> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp
> b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> index d0475fb538a..0152fe6d7ff 100644
> --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> @@ -103,7 +103,7 @@ st_nir_assign_vs_in_locations(struct gl_program *prog,
> nir_shader *nir)
>            * set.
>            */
>           exec_node_remove(&var->node);
> -         var->data.mode = nir_var_global;
> +         var->data.mode = nir_var_private;
>           exec_list_push_tail(&nir->globals, &var->node);
>        }
>     }
> --
> 2.19.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181114/67312806/attachment-0001.html>


More information about the mesa-dev mailing list