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

[Samuel Pitoiset]

On 02/24/2017 10:07 AM, Andres Gomez wrote:
> 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:

Okay, I will update locally before pushing.
Thanks!

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