[Mesa-dev] [PATCH] glsl: use an enum for AMD_conservative_depth layout qualifiers

[Andres Gomez]

On Thu, 2017-02-23 at 18:07 +0100, Samuel Pitoiset wrote:
> The main idea behind this is to free some bits in the flags.q
> struct because currently all 64-bits are used and we can't
> add more layout qualifiers without reaching a static assert.
> 
> In order to do that (mainly for ARB_bindless_texture), use an
> enumeration for the AMD_conservative_depth layout qualifiers
> because it's forbidden to declare more than one depth qualifier
> for gl_FragDepth.
> 
> Note that ast_type_qualifier::merge_qualifier() will prevent
> using duplicate layout qualifiers by returning a compile-time
> error.
> 
> No piglit regressions found (including compiler tests) with
> RX480 on RadeonSI.
> 
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
> ---
>  src/compiler/glsl/ast.h          | 16 ++++++++++++----
>  src/compiler/glsl/ast_to_hir.cpp | 21 ++++++---------------
>  src/compiler/glsl/ast_type.cpp   | 12 +++---------
>  src/compiler/glsl/glsl_parser.yy | 12 ++++++++----
>  4 files changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> index 3dff164232..11a092e41c 100644
> --- a/src/compiler/glsl/ast.h
> +++ b/src/compiler/glsl/ast.h
> @@ -463,6 +463,14 @@ enum {
>     ast_precision_low
>  };
>  
> +enum {
> +   ast_depth_none = 0, /**< Absence of depth qualifier. */
> +   ast_depth_any,
> +   ast_depth_greater,
> +   ast_depth_less,
> +   ast_depth_unchanged
> +};
> +
>  struct ast_type_qualifier {
>     DECLARE_RALLOC_CXX_OPERATORS(ast_type_qualifier);
>  
> @@ -528,10 +536,7 @@ struct ast_type_qualifier {
>  
>           /** \name Layout qualifiers for GL_AMD_conservative_depth */
>           /** \{ */
> -         unsigned depth_any:1;
> -         unsigned depth_greater:1;
> -         unsigned depth_less:1;
> -         unsigned depth_unchanged:1;
> +         unsigned depth_type:1;
>           /** \} */
>  
>  	 /** \name Layout qualifiers for GL_ARB_uniform_buffer_object */
> @@ -630,6 +635,9 @@ struct ast_type_qualifier {
>     /** Precision of the type (highp/medium/lowp). */
>     unsigned precision:2;
>  
> +   /** Type of layout qualifiers for GL_AMD_conservative_depth. */
> +   unsigned depth_type:3;
> +
>     /**
>      * Alignment specified via GL_ARB_enhanced_layouts "align" layout qualifier
>      */
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index 88febe7ff3..ad1fd9150a 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -3629,11 +3629,7 @@ apply_layout_qualifier_to_variable(const struct ast_type_qualifier *qual,
>     /* Layout qualifiers for gl_FragDepth, which are enabled by extension
>      * AMD_conservative_depth.
>      */
> -   int depth_layout_count = qual->flags.q.depth_any
> -      + qual->flags.q.depth_greater
> -      + qual->flags.q.depth_less
> -      + qual->flags.q.depth_unchanged;
> -   if (depth_layout_count > 0
> +   if (qual->flags.q.depth_type
>         && !state->is_version(420, 0)
>         && !state->AMD_conservative_depth_enable
>         && !state->ARB_conservative_depth_enable) {
> @@ -3641,24 +3637,19 @@ apply_layout_qualifier_to_variable(const struct ast_type_qualifier *qual,
>                          "extension GL_AMD_conservative_depth or "
>                          "GL_ARB_conservative_depth must be enabled "
>                          "to use depth layout qualifiers");
> -   } else if (depth_layout_count > 0
> +   } else if (qual->flags.q.depth_type
>                && strcmp(var->name, "gl_FragDepth") != 0) {
>         _mesa_glsl_error(loc, state,
>                          "depth layout qualifiers can be applied only to "
>                          "gl_FragDepth");
> -   } else if (depth_layout_count > 1
> -              && strcmp(var->name, "gl_FragDepth") == 0) {
> -      _mesa_glsl_error(loc, state,
> -                       "at most one depth layout qualifier can be applied to "
> -                       "gl_FragDepth");
>     }
> -   if (qual->flags.q.depth_any)
> +   if (qual->depth_type == ast_depth_any)
>        var->data.depth_layout = ir_depth_layout_any;
> -   else if (qual->flags.q.depth_greater)
> +   else if (qual->depth_type == ast_depth_greater)
>        var->data.depth_layout = ir_depth_layout_greater;
> -   else if (qual->flags.q.depth_less)
> +   else if (qual->depth_type == ast_depth_less)
>        var->data.depth_layout = ir_depth_layout_less;
> -   else if (qual->flags.q.depth_unchanged)
> +   else if (qual->depth_type == ast_depth_unchanged)
>         var->data.depth_layout = ir_depth_layout_unchanged;
>     else
>         var->data.depth_layout = ir_depth_layout_none;

Maybe replace this with a ?

switch(qual->depth_type) {
case ast_depth_any:
...
}

Other than that nitpick, this is:

Reviewed-by: Andres Gomez <agomez at igalia.com>
-- 
Br,

Andres