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.jus...@intel.com>
> > Cc: Ilia Mirkin <imir...@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 (apinhe...@igalia.com)
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
