[Mesa-dev] [PATCH v4 02/14] mesa/main: add support for ARB_compute_variable_groups_size

[Nicolai Hähnle]

On 05.10.2016 20:48, Samuel Pitoiset wrote:
> v4: - slightly indent spec quotes (Nicolai)
>     - drop useless _mesa_has_compute_shaders() check (Nicolai)
>     - move the fixed local size outside of the loop (Nicolai)
>     - add missing check for invalid use of work group count
> v2: - update formatting spec quotations (Ian)
>     - move the total_invocations check outside of the loop (Ian)
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>
> fix patch 2
> ---
>  src/mesa/main/api_validate.c     | 111 +++++++++++++++++++++++++++++++++++++++
>  src/mesa/main/api_validate.h     |   4 ++
>  src/mesa/main/compute.c          |  17 ++++++
>  src/mesa/main/context.c          |   6 +++
>  src/mesa/main/dd.h               |   9 ++++
>  src/mesa/main/extensions_table.h |   1 +
>  src/mesa/main/get.c              |  10 ++++
>  src/mesa/main/get_hash_params.py |   3 ++
>  src/mesa/main/mtypes.h           |  24 ++++++++-
>  src/mesa/main/shaderapi.c        |   1 +
>  src/mesa/main/shaderobj.c        |   2 +
>  11 files changed, 187 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
> index 05691d2..341c40e 100644
> --- a/src/mesa/main/api_validate.c
> +++ b/src/mesa/main/api_validate.c
> @@ -1285,6 +1285,7 @@ GLboolean
>  _mesa_validate_DispatchCompute(struct gl_context *ctx,
>                                 const GLuint *num_groups)
>  {
> +   struct gl_shader_program *prog;
>     int i;
>     FLUSH_CURRENT(ctx, 0);
>
> @@ -1317,6 +1318,103 @@ _mesa_validate_DispatchCompute(struct gl_context *ctx,
>        }
>     }
>
> +   /* The ARB_compute_variable_group_size spec says:
> +    *
> +    * "An INVALID_OPERATION error is generated by DispatchCompute if the active
> +    *  program for the compute shader stage has a variable work group size."
> +    */
> +   prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
> +   if (prog->Comp.LocalSizeVariable) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION,
> +                  "glDispatchCompute(variable work group size forbidden)");
> +      return GL_FALSE;
> +   }
> +
> +   return GL_TRUE;
> +}
> +
> +GLboolean
> +_mesa_validate_DispatchComputeGroupSizeARB(struct gl_context *ctx,
> +                                           const GLuint *num_groups,
> +                                           const GLuint *group_size)
> +{
> +   struct gl_shader_program *prog;
> +   GLuint total_invocations = 1;
> +   GLuint fixed_local_size = 0;
> +   int i;
> +
> +   FLUSH_CURRENT(ctx, 0);
> +
> +   if (!check_valid_to_compute(ctx, "glDispatchComputeGroupSizeARB"))
> +      return GL_FALSE;
> +
> +   prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
> +   for (i = 0; i < 3; i++) {
> +      /* The ARB_compute_variable_group_size spec says:
> +       *
> +       * "An INVALID_VALUE error is generated if any of num_groups_x,
> +       *  num_groups_y and num_groups_z are greater than or equal to the
> +       *  maximum work group count for the corresponding dimension."
> +       */
> +      if (num_groups[i] > ctx->Const.MaxComputeWorkGroupCount[i]) {
> +         _mesa_error(ctx, GL_INVALID_VALUE,
> +                     "glDispatchComputeGroupSizeARB(num_groups_%c)", 'x' + i);
> +         return GL_FALSE;
> +      }
> +
> +      /* The ARB_compute_variable_group_size spec says:
> +       *
> +       * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB if
> +       *  any of <group_size_x>, <group_size_y>, or <group_size_z> is less than
> +       *  or equal to zero or greater than the maximum local work group size
> +       *  for compute shaders with variable group size
> +       *  (MAX_COMPUTE_VARIABLE_GROUP_SIZE_ARB) in the corresponding
> +       *  dimension."
> +       *
> +       * However, the "less than" is a spec bug because they are declared as
> +       * unsigned integers.
> +       */
> +      if (group_size[i] == 0 ||
> +          group_size[i] > ctx->Const.MaxComputeVariableGroupSize[i]) {
> +         _mesa_error(ctx, GL_INVALID_VALUE,
> +                     "glDispatchComputeGroupSizeARB(group_size_%c)", 'x' + i);
> +         return GL_FALSE;
> +      }
> +
> +      fixed_local_size  += prog->Comp.LocalSize[i];
> +      total_invocations *= group_size[i];
> +   }
> +
> +   /* The ARB_compute_variable_group_size spec says:
> +    *
> +    * "An INVALID_OPERATION error is generated by
> +    *  DispatchComputeGroupSizeARB if the active program for the compute
> +    *  shader stage has a fixed work group size."
> +    */
> +   if (fixed_local_size > 0) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION,
> +                  "glDispatchComputeGroupSizeARB(fixed work group size "
> +                  "forbidden)");
> +      return GL_FALSE;
> +   }

I think this test should just be if (!prog->Comp.LocalSizeVariable), and 
it should go above the loop (as a consequence, fixed_local_size can be 
removed).

I don't know whether the spec really cares about the order in which 
errors are checked, but the error messages we generate will be less 
confusing that way.

With that changed, the patch is

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>