[Mesa-dev] [PATCH 07/12] glsl: Add arb_cull_distance support

Tobias Klausmann tobias.johannes.klausmann at mni.thm.de
Tue Apr 5 12:54:11 UTC 2016



On 05.04.2016 01:32, Timothy Arceri wrote:
> On Mon, 2016-04-04 at 15:07 +0200, Tobias Klausmann wrote:
>> On 04.04.2016 04:48, Timothy Arceri wrote:
>>> On Mon, 2016-04-04 at 12:15 +1000, Dave Airlie wrote:
>>>> From: Tobias Klausmann <tobias.johannes.klausmann at mni.thm.de>
>>>>
>>>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann at mni.th
>>>> m.de
>>>> ---
>>>>    src/compiler/glsl/ast_to_hir.cpp             |  14 +++
>>>>    src/compiler/glsl/builtin_variables.cpp      |  11 ++-
>>>>    src/compiler/glsl/glcpp/glcpp-parse.y        |   3 +
>>>>    src/compiler/glsl/glsl_parser_extras.cpp     |   1 +
>>>>    src/compiler/glsl/glsl_parser_extras.h       |   2 +
>>>>    src/compiler/glsl/link_varyings.cpp          |  10 +++
>>>>    src/compiler/glsl/link_varyings.h            |   1 +
>>>>    src/compiler/glsl/linker.cpp                 | 122
>>>> +++++++++++++++++++++------
>>>>    src/compiler/glsl/standalone_scaffolding.cpp |   1 +
>>>>    src/compiler/glsl/tests/varyings_test.cpp    |  27 ++++++
>>>>    src/compiler/shader_enums.h                  |   4 +
>>>>    11 files changed, 169 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>>>> b/src/compiler/glsl/ast_to_hir.cpp
>>>> index 7c9be81..47db841 100644
>>>> --- a/src/compiler/glsl/ast_to_hir.cpp
>>>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>>>> @@ -1210,6 +1210,20 @@ check_builtin_array_max_size(const char
>>>> *name,
>>>> unsigned size,
>>>>          _mesa_glsl_error(&loc, state, "`gl_ClipDistance' array
>>>> size
>>>> cannot "
>>>>                           "be larger than gl_MaxClipDistances
>>>> (%u)",
>>>>                           state->Const.MaxClipPlanes);
>>>> +   } else if (strcmp("gl_CullDistance", name) == 0
>>>> +              && size > state->Const.MaxClipPlanes) {
>>>> +      /* From the ARB_cull_distance spec:
>>>> +       *
>>>> +       *   "The gl_CullDistance array is predeclared as unsized
>>>> and
>>>> +       *    must be sized by the shader either redeclaring it
>>>> with
>>>> +       *    a size or indexing it only with integral constant
>>>> +       *    expressions. The size determines the number and set
>>>> of
>>>> +       *    enabled cull distances and can be at most
>>>> +       *    gl_MaxCullDistances."
>>>> +       */
>>>> +      _mesa_glsl_error(&loc, state, "`gl_CullDistance' array
>>>> size
>>>> cannot "
>>>> +                       "be larger than gl_MaxCullDistances
>>>> (%u)",
>>>> +                       state->Const.MaxClipPlanes);
>>>>       }
>>>>    }
>>>>    
>>>> diff --git a/src/compiler/glsl/builtin_variables.cpp
>>>> b/src/compiler/glsl/builtin_variables.cpp
>>>> index f31f9f6..8d049c8 100644
>>>> --- a/src/compiler/glsl/builtin_variables.cpp
>>>> +++ b/src/compiler/glsl/builtin_variables.cpp
>>>> @@ -302,7 +302,7 @@ public:
>>>>       const glsl_type *construct_interface_instance() const;
>>>>    
>>>>    private:
>>>> -   glsl_struct_field fields[10];
>>>> +   glsl_struct_field fields[11];
>>>>       unsigned num_fields;
>>>>    };
>>>>    
>>>> @@ -675,6 +675,11 @@
>>>> builtin_variable_generator::generate_constants()
>>>>          add_const("gl_MaxClipDistances", state-
>>>>> Const.MaxClipPlanes);
>>>>          add_const("gl_MaxVaryingComponents", state->ctx-
>>>>> Const.MaxVarying * 4);
>>>>       }
>>>> +   if (state->is_version(450, 0) || state-
>>>>> ARB_cull_distance_enable)
>>>> {
>>>> +      add_const("gl_MaxCullDistances", state-
>>>>> Const.MaxClipPlanes);
>>>> +      add_const("gl_MaxCombinedClipAndCullDistances",
>>>> +                state->Const.MaxClipPlanes);
>>>> +   }
>>>>    
>>>>       if (state->has_geometry_shader()) {
>>>>          add_const("gl_MaxVertexOutputComponents",
>>>> @@ -1246,6 +1251,10 @@
>>>> builtin_variable_generator::generate_varyings()
>>>>           add_varying(VARYING_SLOT_CLIP_DIST0, array(float_t, 0),
>>>>                       "gl_ClipDistance");
>>>>       }
>>>> +   if (state->is_version(450, 0) || state-
>>>>> ARB_cull_distance_enable)
>>>> {
>>>> +      add_varying(VARYING_SLOT_CULL_DIST0, array(float_t, 0),
>>>> +                   "gl_CullDistance");
>>>> +   }
>>>>    
>>>>       if (compatibility) {
>>>>          add_varying(VARYING_SLOT_TEX0, array(vec4_t, 0),
>>>> "gl_TexCoord");
>>>> diff --git a/src/compiler/glsl/glcpp/glcpp-parse.y
>>>> b/src/compiler/glsl/glcpp/glcpp-parse.y
>>>> index a48266c..e44f074 100644
>>>> --- a/src/compiler/glsl/glcpp/glcpp-parse.y
>>>> +++ b/src/compiler/glsl/glcpp/glcpp-parse.y
>>>> @@ -2457,6 +2457,9 @@
>>>> _glcpp_parser_handle_version_declaration(glcpp_parser_t *parser,
>>>> intmax_t versio
>>>>    
>>>>             if (extensions->ARB_shader_draw_parameters)
>>>>                add_builtin_define(parser,
>>>> "GL_ARB_shader_draw_parameters", 1);
>>>> +
>>>> +         if (extensions->ARB_cull_distance)
>>>> +            add_builtin_define(parser, "GL_ARB_cull_distance",
>>>> 1);
>>>>          }
>>>>       }
>>>>    
>>>> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
>>>> b/src/compiler/glsl/glsl_parser_extras.cpp
>>>> index 76321aa..9b1d53f 100644
>>>> --- a/src/compiler/glsl/glsl_parser_extras.cpp
>>>> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
>>>> @@ -569,6 +569,7 @@ static const _mesa_glsl_extension
>>>> _mesa_glsl_supported_extensions[] = {
>>>>       EXT(ARB_arrays_of_arrays,             true,  false,     ARB_
>>>> array
>>>> s_of_arrays),
>>>>       EXT(ARB_compute_shader,               true,  false,     ARB_
>>>> compu
>>>> te_shader),
>>>>       EXT(ARB_conservative_depth,           true,  false,     ARB_
>>>> conse
>>>> rvative_depth),
>>>> +   EXT(ARB_cull_distance,                true,  false,     ARB_c
>>>> ull_
>>>> distance),
>>>>       EXT(ARB_derivative_control,           true,  false,     ARB_
>>>> deriv
>>>> ative_control),
>>>>       EXT(ARB_draw_buffers,                 true,  false,     dumm
>>>> y_tru
>>>> e),
>>>>       EXT(ARB_draw_instanced,               true,  false,     ARB_
>>>> draw_
>>>> instanced),
>>>> diff --git a/src/compiler/glsl/glsl_parser_extras.h
>>>> b/src/compiler/glsl/glsl_parser_extras.h
>>>> index c774fbe..85a8ebf 100644
>>>> --- a/src/compiler/glsl/glsl_parser_extras.h
>>>> +++ b/src/compiler/glsl/glsl_parser_extras.h
>>>> @@ -518,6 +518,8 @@ struct _mesa_glsl_parse_state {
>>>>       bool ARB_compute_shader_warn;
>>>>       bool ARB_conservative_depth_enable;
>>>>       bool ARB_conservative_depth_warn;
>>>> +   bool ARB_cull_distance_enable;
>>>> +   bool ARB_cull_distance_warn;
>>>>       bool ARB_derivative_control_enable;
>>>>       bool ARB_derivative_control_warn;
>>>>       bool ARB_draw_buffers_enable;
>>>> diff --git a/src/compiler/glsl/link_varyings.cpp
>>>> b/src/compiler/glsl/link_varyings.cpp
>>>> index 8e74981..d4cc68f 100644
>>>> --- a/src/compiler/glsl/link_varyings.cpp
>>>> +++ b/src/compiler/glsl/link_varyings.cpp
>>>> @@ -573,6 +573,10 @@ tfeedback_decl::init(struct gl_context *ctx,
>>>> const void *mem_ctx,
>>>>           strcmp(this->var_name, "gl_ClipDistance") == 0) {
>>>>          this->lowered_builtin_array_variable = clip_distance;
>>>>       }
>>>> +   if (ctx-
>>>>> Const.ShaderCompilerOptions[MESA_SHADER_VERTEX].LowerCombinedCl
>>>>> ipCul
>>>> lDistance &&
>>>> +       strcmp(this->var_name, "gl_CullDistance") == 0) {
>>>> +      this->lowered_builtin_array_variable = cull_distance;
>>>> +   }
>>>>    
>>>>       if (ctx->Const.LowerTessLevel &&
>>>>           (strcmp(this->var_name, "gl_TessLevelOuter") == 0))
>>>> @@ -633,6 +637,9 @@ tfeedback_decl::assign_location(struct
>>>> gl_context
>>>> *ctx,
>>>>          case clip_distance:
>>>>             actual_array_size = prog->LastClipDistanceArraySize;
>>>>             break;
>>>> +      case cull_distance:
>>>> +         actual_array_size = prog->LastCullDistanceArraySize;
>>>> +         break;
>>>>          case tess_level_outer:
>>>>             actual_array_size = 4;
>>>>             break;
>>>> @@ -853,6 +860,9 @@
>>>> tfeedback_decl::find_candidate(gl_shader_program
>>>> *prog,
>>>>       case clip_distance:
>>>>          name = "gl_ClipDistanceMESA";
>>>>          break;
>>>> +   case cull_distance:
>>>> +      name = "gl_CullDistanceMESA";
>>>> +      break;
>>>>       case tess_level_outer:
>>>>          name = "gl_TessLevelOuterMESA";
>>>>          break;
>>>> diff --git a/src/compiler/glsl/link_varyings.h
>>>> b/src/compiler/glsl/link_varyings.h
>>>> index 543b80f..0ad4f74 100644
>>>> --- a/src/compiler/glsl/link_varyings.h
>>>> +++ b/src/compiler/glsl/link_varyings.h
>>>> @@ -210,6 +210,7 @@ private:
>>>>       enum {
>>>>          none,
>>>>          clip_distance,
>>>> +      cull_distance,
>>>>          tess_level_outer,
>>>>          tess_level_inner,
>>>>       } lowered_builtin_array_variable;
>>>> diff --git a/src/compiler/glsl/linker.cpp
>>>> b/src/compiler/glsl/linker.cpp
>>>> index 9acf253..77142aa 100644
>>>> --- a/src/compiler/glsl/linker.cpp
>>>> +++ b/src/compiler/glsl/linker.cpp
>>>> @@ -641,19 +641,25 @@
>>>> link_invalidate_variable_locations(exec_list
>>>> *ir)
>>>>    
>>>>    
>>>>    /**
>>>> - * Set clip_distance_array_size based on the given shader.
>>>> + * Set clip_distance_array_size based and
>>>> cull_distance_array_size
>>>> on the given
>>>> + * shader.
>>>>     *
>>>>     * Also check for errors based on incorrect usage of
>>>> gl_ClipVertex
>>>> and
>>>> - * gl_ClipDistance.
>>>> + * gl_ClipDistance and gl_CullDistance.
>>>> + * Additionally test whether the arrays gl_ClipDistance and
>>>> gl_CullDistance
>>>> + * exceed the maximum size defined by
>>>> gl_MaxCombinedClipAndCullDistances.
>>>>     *
>>>>     * Return false if an error was reported.
>>>>     */
>>>>    static void
>>>> -analyze_clip_usage(struct gl_shader_program *prog,
>>>> -                   struct gl_shader *shader,
>>>> -                   GLuint *clip_distance_array_size)
>>>> +analyze_clip_cull_usage(struct gl_shader_program *prog,
>>>> +                        struct gl_shader *shader,
>>>> +                        struct gl_context *ctx,
>>>> +                        GLuint *clip_distance_array_size,
>>>> +                        GLuint *cull_distance_array_size)
>>>>    {
>>>>       *clip_distance_array_size = 0;
>>>> +   *cull_distance_array_size = 0;
>>>>    
>>>>       if (!prog->IsES && prog->Version >= 130) {
>>>>          /* From section 7.1 (Vertex Shader Special Variables) of
>>>> the
>>>> @@ -667,22 +673,71 @@ analyze_clip_usage(struct gl_shader_program
>>>> *prog,
>>>>           */
>>>>          find_assignment_visitor clip_vertex("gl_ClipVertex");
>>>>          find_assignment_visitor clip_distance("gl_ClipDistance");
>>>> +      find_assignment_visitor cull_distance("gl_CullDistance");
>>>>    
>>>>          clip_vertex.run(shader->ir);
>>>>          clip_distance.run(shader->ir);
>>>> +      cull_distance.run(shader->ir);
>>>> +
>>>> +      /* From the ARB_cull_distance spec:
>>>> +       *
>>>> +       * It is a compile-time or link-time error for the set of
>>>> shaders forming
>>>> +       * a program to statically read or write both
>>>> gl_ClipVertex
>>>> and either
>>>> +       * gl_ClipDistance or gl_CullDistance.
>>>> +       *
>>>> +       * This does not apply to GLSL ES shaders, since GLSL ES
>>>> defines neither
>>>> +       * gl_ClipVertex, gl_ClipDistance or gl_CullDistance.
>>>> +       */
>>>>          if (clip_vertex.variable_found() &&
>>>> clip_distance.variable_found()) {
>>>>             linker_error(prog, "%s shader writes to both
>>>> `gl_ClipVertex' "
>>>>                          "and `gl_ClipDistance'\n",
>>>>                          _mesa_shader_stage_to_string(shader-
>>>>> Stage));
>>>>             return;
>>>>          }
>>>> +      if (clip_vertex.variable_found() &&
>>>> cull_distance.variable_found()) {
>>>> +         linker_error(prog, "%s shader writes to both
>>>> `gl_ClipVertex' "
>>>> +                      "and `gl_CullDistance'\n",
>>>> +                      _mesa_shader_stage_to_string(shader-
>>>>> Stage));
>>>> +         return;
>>>> +      }
>>>>    
>>>>          if (clip_distance.variable_found()) {
>>>>             ir_variable *clip_distance_var =
>>>> -               shader->symbols->get_variable("gl_ClipDistance");
>>>> -
>>>> +                shader->symbols-
>>>>> get_variable("gl_ClipDistance");
>>>>             assert(clip_distance_var);
>>>> -         *clip_distance_array_size = clip_distance_var->type-
>>>>> length;
>>>> +         if (clip_distance_var->type->was_unsized)
>>>> +            *clip_distance_array_size =
>>>> clip_distance.variable_found() ?
>>>> +                                        clip_distance_var->type-
>>>>> length :
>>>> +                                        clip_distance_var->type-
>>>>> length -1;
>>>> +         else
>>>> +            *clip_distance_array_size = clip_distance_var->type-
>>>>> length;
>>>> +      }
>>>> +      if (cull_distance.variable_found()) {
>>>> +         ir_variable *cull_distance_var =
>>>> +                shader->symbols-
>>>>> get_variable("gl_CullDistance");
>>>> +        assert(cull_distance_var);
>>>> +          if (cull_distance_var->type->was_unsized)
>>>> +             *cull_distance_array_size =
>>>> cull_distance.variable_found() ?
>>>> +                                         cull_distance_var-
>>>>> type-
>>>>>
>>>>> length :
>>>> +                                         cull_distance_var-
>>>>> type-
>>>>>
>>>>> length -1;
>>>> +          else
>>>> +              *cull_distance_array_size = cull_distance_var-
>>>>> type-
>>>>>
>>>>> length;
>>> I think we need a few piglit test cases for
>>> the gl_MaxCombinedClipAndCullDistance test below. The above code
>>> makes
>>> no sense.
>>>
>>> We are inside an if cull_distance.variable_found()
>>>
>>> Then we check for cull_distance_var->type->was_unsized
>>>
>>> and if cull_distance.variable_found() we do:
>>>
>>> *cull_distance_array_size = cull_distance_var->type->length;
>>>
>>> So in otherwords we always just do
>>>
>>> *cull_distance_array_size = cull_distance_var->type->length;
>>>
>>> The was_unsized makes no difference same goes for the clip_distance
>>> changes above. I don't think this is intended.
>>>
>> It was intended back then to fix an off-by-one for the combined
>> size,
>> but i can't remember the actual test i fixed with that (there are 5
>> additional piglit tests never got up-streamed for cull-distance,
>> maybe
>> it was one of those i'll check when i got some time)
> It looks like it was trying to fix the issues from our discussion last
> year [1]. Since it now skips the original change you were trying to
> make I can only asssume there are no piglit tests. I haven't check this
> however.
>
> [1] https://lists.freedesktop.org/archives/mesa-dev/2015-May/084992.htm
> l
>

Oh, where do i have my mind! You are right, we need tests for this case 
to be sure.
Thanks,
Tobias


More information about the mesa-dev mailing list