[Mesa-dev] [PATCH v2 3/3] glsl/cs: Initialize gl_GlobalInvocationID in main()

[Jordan Justen]

On 2015-09-10 05:48:50, Alejandro Piñeiro wrote:
> Not a full review, but a comment. See inline.
> 
> On 23/08/15 09:09, Jordan Justen wrote:
> > We initialize gl_GlobalInvocationID based on the extension spec
> > formula:
> >
> >     gl_GlobalInvocationID =
> >         gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID
> >
> > https://www.opengl.org/registry/specs/ARB/compute_shader.txt
> >
> > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> > ---
> >  src/glsl/builtin_variables.cpp  | 58 +++++++++++++++++++++++++++++++++++++++++
> >  src/glsl/glsl_parser_extras.cpp |  2 ++
> >  src/glsl/ir.h                   |  3 +++
> >  3 files changed, 63 insertions(+)
> >
> > diff --git a/src/glsl/builtin_variables.cpp b/src/glsl/builtin_variables.cpp
> > index 5d9e446..8f8be90 100644
> > --- a/src/glsl/builtin_variables.cpp
> > +++ b/src/glsl/builtin_variables.cpp
> > @@ -22,6 +22,8 @@
> >   */
> >  
> >  #include "ir.h"
> > +#include "ir_builder.h"
> > +#include "linker.h"
> >  #include "glsl_parser_extras.h"
> >  #include "glsl_symbol_table.h"
> >  #include "main/core.h"
> > @@ -1056,6 +1058,7 @@ builtin_variable_generator::generate_cs_special_vars()
> >                      "gl_LocalInvocationID");
> >     add_system_value(SYSTEM_VALUE_WORK_GROUP_ID, glsl_type::uvec3_type,
> >                      "gl_WorkGroupID");
> > +   add_variable("gl_GlobalInvocationID", glsl_type::uvec3_type, ir_var_auto, 0);
> >     /* TODO: finish this. */
> >  }
> >  
> > @@ -1212,3 +1215,58 @@ _mesa_glsl_initialize_variables(exec_list *instructions,
> >        break;
> >     }
> >  }
> > +
> > +
> > +/**
> > + * Initialize compute shader variables with values that are derived from other
> > + * compute shader variable.
> > + */
> > +static void
> > +initialize_cs_derived_variables(gl_shader *shader,
> > +                                ir_function_signature *const main_sig)
> > +{
> > +   assert(shader->Stage == MESA_SHADER_COMPUTE);
> > +
> > +   ir_variable *gl_GlobalInvocationID =
> > +      shader->symbols->get_variable("gl_GlobalInvocationID");
> > +   assert(gl_GlobalInvocationID);
> > +   ir_variable *gl_WorkGroupID =
> > +      shader->symbols->get_variable("gl_WorkGroupID");
> > +   assert(gl_WorkGroupID);
> > +   ir_variable *gl_WorkGroupSize =
> > +      shader->symbols->get_variable("gl_WorkGroupSize");
> > +   assert(gl_WorkGroupSize);
> 
> This assert seems somewhat too restrictive at this point. After this
> commit, the following piglit tests are failing:
>   *
> http://cgit.freedesktop.org/piglit/tree/tests/spec/arb_compute_shader/compiler/gl_WorkGroupSize_without_layout.comp
>   *
> http://cgit.freedesktop.org/piglit/tree/tests/spec/arb_compute_shader/linker/no_local_work_size.shader_test
> 
> The correct outcome for both tests are an error (compile and link error
> respectively). But the assert causes it to crash.

Good catch. I had to replace this assert with:

   if (gl_WorkGroupSize == NULL) {
      void *const mem_ctx = ralloc_parent(shader->ir);
      gl_WorkGroupSize = new(mem_ctx) ir_variable(glsl_type::uvec3_type,
                                                  "gl_WorkGroupSize",
                                                  ir_var_auto);
      gl_WorkGroupSize->data.how_declared = ir_var_declared_implicitly;
      gl_WorkGroupSize->data.read_only = true;
      shader->ir->push_head(gl_WorkGroupSize);
   }

-Jordan

> 
> > +   ir_variable *gl_LocalInvocationID =
> > +      shader->symbols->get_variable("gl_LocalInvocationID");
> > +   assert(gl_LocalInvocationID);
> > +
> > +   /* gl_GlobalInvocationID =
> > +    *    gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID
> > +    */
> > +   ir_instruction *inst =
> > +      ir_builder::assign(gl_GlobalInvocationID,
> > +                         ir_builder::add(ir_builder::mul(gl_WorkGroupID,
> > +                                                         gl_WorkGroupSize),
> > +                                         gl_LocalInvocationID));
> > +   main_sig->body.push_head(inst);
> > +}
> > +
> > +
> > +/**
> > + * Initialize builtin variables with values based on other builtin variables.
> > + * These are initialized in the main function.
> > + */
> > +void
> > +_mesa_glsl_initialize_derived_variables(gl_shader *shader)
> > +{
> > +   /* We only need to set CS variables currently. */
> > +   if (shader->Stage != MESA_SHADER_COMPUTE)
> > +      return;
> > +
> > +   ir_function_signature *const main_sig =
> > +      _mesa_get_main_function_signature(shader);
> > +   if (main_sig == NULL)
> > +      return;
> > +
> > +   initialize_cs_derived_variables(shader, main_sig);
> > +}
> > diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> > index 6440a96..eefa12a 100644
> > --- a/src/glsl/glsl_parser_extras.cpp
> > +++ b/src/glsl/glsl_parser_extras.cpp
> > @@ -1692,6 +1692,8 @@ _mesa_glsl_compile_shader(struct gl_context *ctx, struct gl_shader *shader,
> >        }
> >     }
> >  
> > +   _mesa_glsl_initialize_derived_variables(shader);
> > +
> >     delete state->symbols;
> >     ralloc_free(state);
> >  }
> > diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> > index 750321e..4c88144 100644
> > --- a/src/glsl/ir.h
> > +++ b/src/glsl/ir.h
> > @@ -2513,6 +2513,9 @@ _mesa_glsl_initialize_variables(exec_list *instructions,
> >                               struct _mesa_glsl_parse_state *state);
> >  
> >  extern void
> > +_mesa_glsl_initialize_derived_variables(gl_shader *shader);
> > +
> > +extern void
> >  _mesa_glsl_initialize_functions(_mesa_glsl_parse_state *state);
> >  
> >  extern void
> 
> -- 
> Alejandro Piñeiro (apinheiro at igalia.com)
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev