[Mesa-dev] [PATCH v3 06/11] glsl: Add arb_cull_distance support
Ian Romanick
idr at freedesktop.org
Mon May 9 18:41:58 UTC 2016
On 05/08/2016 10:44 PM, Tobias Klausmann wrote:
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann at mni.thm.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 | 111 +++++++++++++++++++++------
> src/compiler/glsl/standalone_scaffolding.cpp | 1 +
> src/compiler/glsl/tests/varyings_test.cpp | 27 +++++++
> src/compiler/shader_enums.h | 4 +
> 11 files changed, 159 insertions(+), 26 deletions(-)
>
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index c5cd48f..a8349da 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);
> }
Blank line here.
> + if (state->is_version(450, 0) || state->ARB_cull_distance_enable) {
In these cases we should add a method to encapsulate this check. See
hsa_atomic_counters() for an example.
> + 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");
> }
Blank line here.
> + 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 fb64350..03a828b 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -576,6 +576,7 @@ static const _mesa_glsl_extension _mesa_glsl_supported_extensions[] = {
> EXT(ARB_arrays_of_arrays, true, false, ARB_arrays_of_arrays),
> EXT(ARB_compute_shader, true, false, ARB_compute_shader),
> EXT(ARB_conservative_depth, true, false, ARB_conservative_depth),
> + EXT(ARB_cull_distance, true, false, ARB_cull_distance),
> EXT(ARB_derivative_control, true, false, ARB_derivative_control),
> EXT(ARB_draw_buffers, true, false, dummy_true),
> 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 7018347..1712b54 100644
> --- a/src/compiler/glsl/glsl_parser_extras.h
> +++ b/src/compiler/glsl/glsl_parser_extras.h
> @@ -520,6 +520,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 2555cc9..003b9d4 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -631,6 +631,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;
> }
Blank line here.
> + if (ctx->Const.ShaderCompilerOptions[MESA_SHADER_VERTEX].LowerCombinedClipCullDistance &&
> + 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))
> @@ -691,6 +695,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;
> @@ -911,6 +918,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 a9c5ab87..36c8c57 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,23 +673,62 @@ 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.
The quoted spec text should be quoted and indented (as in other places
in this patch).
> + */
> 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;
> }
Blank line here.
> + 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");
Spurious whitespace change.
> assert(clip_distance_var);
> *clip_distance_array_size = clip_distance_var->type->length;
> }
Blank line here.
> + if (cull_distance.variable_found()) {
> + ir_variable *cull_distance_var =
> + shader->symbols->get_variable("gl_CullDistance");
> + assert(cull_distance_var);
> + *cull_distance_array_size = cull_distance_var->type->length;
> + }
Blank line here.
> + /* From the ARB_cull_distance spec:
> + *
> + * It is a compile-time or link-time error for the set of shaders forming
> + * a program to have the sum of the sizes of the gl_ClipDistance and
> + * gl_CullDistance arrays to be larger than
> + * gl_MaxCombinedClipAndCullDistances.
> + */
> + if ((*clip_distance_array_size + *cull_distance_array_size) >
> + ctx->Const.MaxClipPlanes) {
> + linker_error(prog, "%s shader: the combined size of "
> + "'gl_ClipDistance' and 'gl_CullDistance' size cannot "
> + "be larger than "
> + "gl_MaxCombinedClipAndCullDistances (%u)",
> + _mesa_shader_stage_to_string(shader->Stage),
> + ctx->Const.MaxClipPlanes);
> + }
I think this is the wrong check for this limitation in the spec. This
will only catch cases where the usages within a stage exceed the limit.
I *believe* (but it's not 100% clear from the quotation) that a vertex
shader that writes MaxClipPlanes elements to gl_ClipDisatnce and a
geometry shader that also writes MaxClipPlances to gl_CullDistance
should fail to link. If that is the case, then we need checks elsewhere
that use LastClipDistanceArraySize and LastCullDistanceArray size to
enforce the limits.
> }
> }
>
> @@ -691,13 +736,15 @@ analyze_clip_usage(struct gl_shader_program *prog,
> /**
> * Verify that a vertex shader executable meets all semantic requirements.
> *
> - * Also sets prog->Vert.ClipDistanceArraySize as a side effect.
> + * Also sets prog->Vert.ClipDistanceArraySize and
> + * prog->Vert.CullDistanceArraySize as a side effect.
> *
> * \param shader Vertex shader executable to be verified
> */
> void
> validate_vertex_shader_executable(struct gl_shader_program *prog,
> - struct gl_shader *shader)
> + struct gl_shader *shader,
> + struct gl_context *ctx)
> {
> if (shader == NULL)
> return;
> @@ -744,17 +791,22 @@ validate_vertex_shader_executable(struct gl_shader_program *prog,
> }
> }
>
> - analyze_clip_usage(prog, shader, &prog->Vert.ClipDistanceArraySize);
> + analyze_clip_cull_usage(prog, shader, ctx,
> + &prog->Vert.ClipDistanceArraySize,
> + &prog->Vert.CullDistanceArraySize);
> }
>
> void
> validate_tess_eval_shader_executable(struct gl_shader_program *prog,
> - struct gl_shader *shader)
> + struct gl_shader *shader,
> + struct gl_context *ctx)
> {
> if (shader == NULL)
> return;
>
> - analyze_clip_usage(prog, shader, &prog->TessEval.ClipDistanceArraySize);
> + analyze_clip_cull_usage(prog, shader, ctx,
> + &prog->TessEval.ClipDistanceArraySize,
> + &prog->TessEval.CullDistanceArraySize);
> }
>
>
> @@ -765,7 +817,7 @@ validate_tess_eval_shader_executable(struct gl_shader_program *prog,
> */
> void
> validate_fragment_shader_executable(struct gl_shader_program *prog,
> - struct gl_shader *shader)
> + struct gl_shader *shader)
> {
> if (shader == NULL)
> return;
> @@ -785,14 +837,15 @@ validate_fragment_shader_executable(struct gl_shader_program *prog,
> /**
> * Verify that a geometry shader executable meets all semantic requirements
> *
> - * Also sets prog->Geom.VerticesIn, and prog->Geom.ClipDistanceArraySize as
> - * a side effect.
> + * Also sets prog->Geom.VerticesIn, and prog->Geom.ClipDistanceArraySize and
> + * prog->Geom.CullDistanceArraySize as a side effect.
> *
> * \param shader Geometry shader executable to be verified
> */
> void
> validate_geometry_shader_executable(struct gl_shader_program *prog,
> - struct gl_shader *shader)
> + struct gl_shader *shader,
> + struct gl_context *ctx)
> {
> if (shader == NULL)
> return;
> @@ -800,7 +853,9 @@ validate_geometry_shader_executable(struct gl_shader_program *prog,
> unsigned num_vertices = vertices_per_prim(prog->Geom.InputType);
> prog->Geom.VerticesIn = num_vertices;
>
> - analyze_clip_usage(prog, shader, &prog->Geom.ClipDistanceArraySize);
> + analyze_clip_cull_usage(prog, shader, ctx,
> + &prog->Geom.ClipDistanceArraySize,
> + &prog->Geom.CullDistanceArraySize);
> }
>
> /**
> @@ -4444,16 +4499,16 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>
> switch (stage) {
> case MESA_SHADER_VERTEX:
> - validate_vertex_shader_executable(prog, sh);
> + validate_vertex_shader_executable(prog, sh, ctx);
> break;
> case MESA_SHADER_TESS_CTRL:
> /* nothing to be done */
> break;
> case MESA_SHADER_TESS_EVAL:
> - validate_tess_eval_shader_executable(prog, sh);
> + validate_tess_eval_shader_executable(prog, sh, ctx);
> break;
> case MESA_SHADER_GEOMETRY:
> - validate_geometry_shader_executable(prog, sh);
> + validate_geometry_shader_executable(prog, sh, ctx);
> break;
> case MESA_SHADER_FRAGMENT:
> validate_fragment_shader_executable(prog, sh);
> @@ -4469,14 +4524,19 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> }
> }
>
> - if (num_shaders[MESA_SHADER_GEOMETRY] > 0)
> + if (num_shaders[MESA_SHADER_GEOMETRY] > 0) {
> prog->LastClipDistanceArraySize = prog->Geom.ClipDistanceArraySize;
> - else if (num_shaders[MESA_SHADER_TESS_EVAL] > 0)
> + prog->LastCullDistanceArraySize = prog->Geom.CullDistanceArraySize;
> + } else if (num_shaders[MESA_SHADER_TESS_EVAL] > 0) {
> prog->LastClipDistanceArraySize = prog->TessEval.ClipDistanceArraySize;
> - else if (num_shaders[MESA_SHADER_VERTEX] > 0)
> + prog->LastCullDistanceArraySize = prog->TessEval.CullDistanceArraySize;
> + } else if (num_shaders[MESA_SHADER_VERTEX] > 0) {
> prog->LastClipDistanceArraySize = prog->Vert.ClipDistanceArraySize;
> - else
> + prog->LastCullDistanceArraySize = prog->Vert.CullDistanceArraySize;
> + } else {
> prog->LastClipDistanceArraySize = 0; /* Not used */
> + prog->LastCullDistanceArraySize = 0; /* Not used */
> + }
>
> /* Here begins the inter-stage linking phase. Some initial validation is
> * performed, then locations are assigned for uniforms, attributes, and
> @@ -4578,7 +4638,8 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> goto done;
>
> if (ctx->Const.ShaderCompilerOptions[i].LowerCombinedClipCullDistance) {
> - lower_combined_clip_cull_distance(prog->_LinkedShaders[i], 0);
> + lower_combined_clip_cull_distance(prog->_LinkedShaders[i],
> + (uint8_t)prog->LastClipDistanceArraySize);
^ space here
> }
>
> if (ctx->Const.LowerTessLevel) {
> diff --git a/src/compiler/glsl/standalone_scaffolding.cpp b/src/compiler/glsl/standalone_scaffolding.cpp
> index 09d7d6e..4edb767 100644
> --- a/src/compiler/glsl/standalone_scaffolding.cpp
> +++ b/src/compiler/glsl/standalone_scaffolding.cpp
> @@ -152,6 +152,7 @@ void initialize_context_to_defaults(struct gl_context *ctx, gl_api api)
> ctx->Extensions.ARB_texture_query_lod = true;
> ctx->Extensions.ARB_uniform_buffer_object = true;
> ctx->Extensions.ARB_viewport_array = true;
> + ctx->Extensions.ARB_cull_distance = true;
>
> ctx->Extensions.OES_EGL_image_external = true;
> ctx->Extensions.OES_standard_derivatives = true;
> diff --git a/src/compiler/glsl/tests/varyings_test.cpp b/src/compiler/glsl/tests/varyings_test.cpp
> index 9be5e83..936f495 100644
> --- a/src/compiler/glsl/tests/varyings_test.cpp
> +++ b/src/compiler/glsl/tests/varyings_test.cpp
> @@ -194,6 +194,33 @@ TEST_F(link_varyings, gl_ClipDistance)
> EXPECT_TRUE(is_empty(consumer_interface_inputs));
> }
>
> +TEST_F(link_varyings, gl_CullDistance)
> +{
> + const glsl_type *const array_8_of_float =
> + glsl_type::get_array_instance(glsl_type::vec(1), 8);
> +
> + ir_variable *const culldistance =
> + new(mem_ctx) ir_variable(array_8_of_float,
> + "gl_CullDistance",
> + ir_var_shader_in);
> +
> + culldistance->data.explicit_location = true;
> + culldistance->data.location = VARYING_SLOT_CULL_DIST0;
> + culldistance->data.explicit_index = 0;
> +
> + ir.push_tail(culldistance);
> +
> + ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
> + &ir,
> + consumer_inputs,
> + consumer_interface_inputs,
> + junk));
> +
> + EXPECT_EQ(culldistance, junk[VARYING_SLOT_CULL_DIST0]);
> + EXPECT_TRUE(is_empty(consumer_inputs));
> + EXPECT_TRUE(is_empty(consumer_interface_inputs));
> +}
> +
> TEST_F(link_varyings, single_interface_input)
> {
> ir_variable *const v =
> diff --git a/src/compiler/shader_enums.h b/src/compiler/shader_enums.h
> index 0c27408..e93345d 100644
> --- a/src/compiler/shader_enums.h
> +++ b/src/compiler/shader_enums.h
> @@ -205,6 +205,8 @@ typedef enum
> VARYING_SLOT_CLIP_VERTEX, /* Does not appear in FS */
> VARYING_SLOT_CLIP_DIST0,
> VARYING_SLOT_CLIP_DIST1,
> + VARYING_SLOT_CULL_DIST0,
> + VARYING_SLOT_CULL_DIST1,
> VARYING_SLOT_PRIMITIVE_ID, /* Does not appear in VS */
> VARYING_SLOT_LAYER, /* Appears as VS or GS output */
> VARYING_SLOT_VIEWPORT, /* Appears as VS or GS output */
> @@ -282,6 +284,8 @@ const char *gl_varying_slot_name(gl_varying_slot slot);
> #define VARYING_BIT_CLIP_VERTEX BITFIELD64_BIT(VARYING_SLOT_CLIP_VERTEX)
> #define VARYING_BIT_CLIP_DIST0 BITFIELD64_BIT(VARYING_SLOT_CLIP_DIST0)
> #define VARYING_BIT_CLIP_DIST1 BITFIELD64_BIT(VARYING_SLOT_CLIP_DIST1)
> +#define VARYING_BIT_CULL_DIST0 BITFIELD64_BIT(VARYING_SLOT_CULL_DIST0)
> +#define VARYING_BIT_CULL_DIST1 BITFIELD64_BIT(VARYING_SLOT_CULL_DIST1)
> #define VARYING_BIT_PRIMITIVE_ID BITFIELD64_BIT(VARYING_SLOT_PRIMITIVE_ID)
> #define VARYING_BIT_LAYER BITFIELD64_BIT(VARYING_SLOT_LAYER)
> #define VARYING_BIT_VIEWPORT BITFIELD64_BIT(VARYING_SLOT_VIEWPORT)
>
More information about the mesa-dev
mailing list