On Mon, 2015-05-25 at 00:46 +0200, Tobias Klausmann wrote:
> hi,
> replay inline.
>
> On 25.05.2015 00:34, Timothy Arceri wrote:
> > On Sun, 2015-05-24 at 19:58 +0200, Tobias Klausmann wrote:
> >> Signed-off-by: Tobias Klausmann <tobias.johannes.klausm...@mni.thm.de>
> >> ---
> >> src/glsl/ast_to_hir.cpp | 14 +++++
> >> src/glsl/builtin_variables.cpp | 13 +++-
> >> src/glsl/glcpp/glcpp-parse.y | 3 +
> >> src/glsl/glsl_parser_extras.cpp | 1 +
> >> src/glsl/glsl_parser_extras.h | 3 +
> >> src/glsl/link_varyings.cpp | 2 +-
> >> src/glsl/linker.cpp | 121
> >> +++++++++++++++++++++++++-----------
> >> src/glsl/standalone_scaffolding.cpp | 1 +
> >> src/glsl/tests/varyings_test.cpp | 27 ++++++++
> >> 9 files changed, 145 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> >> index 8aebb13..4db2b2e 100644
> >> --- a/src/glsl/ast_to_hir.cpp
> >> +++ b/src/glsl/ast_to_hir.cpp
> >> @@ -1045,6 +1045,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/glsl/builtin_variables.cpp
> >> b/src/glsl/builtin_variables.cpp
> >> index 6806aa1..78c8db2 100644
> >> --- a/src/glsl/builtin_variables.cpp
> >> +++ b/src/glsl/builtin_variables.cpp
> >> @@ -298,7 +298,7 @@ public:
> >> const glsl_type *construct_interface_instance() const;
> >>
> >> private:
> >> - glsl_struct_field fields[10];
> >> + glsl_struct_field fields[11];
> >> unsigned num_fields;
> >> };
> >>
> >> @@ -600,6 +600,12 @@ builtin_variable_generator::generate_constants()
> >> 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->is_version(150, 0)) {
> >> add_const("gl_MaxVertexOutputComponents",
> >> state->Const.MaxVertexOutputComponents);
> >> @@ -1029,6 +1035,11 @@ builtin_variable_generator::generate_varyings()
> >> "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");
> >> ADD_VARYING(VARYING_SLOT_FOGC, float_t, "gl_FogFragCoord");
> >> diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
> >> index a11b6b2..536c17f 100644
> >> --- a/src/glsl/glcpp/glcpp-parse.y
> >> +++ b/src/glsl/glcpp/glcpp-parse.y
> >> @@ -2483,6 +2483,9 @@
> >> _glcpp_parser_handle_version_declaration(glcpp_parser_t *parser, intmax_t
> >> versio
> >>
> >> if (extensions->ARB_shader_precision)
> >> add_builtin_define(parser, "GL_ARB_shader_precision",
> >> 1);
> >> +
> >> + if (extensions->ARB_cull_distance)
> >> + add_builtin_define(parser, "GL_ARB_cull_distance", 1);
> >> }
> >> }
> >>
> >> diff --git a/src/glsl/glsl_parser_extras.cpp
> >> b/src/glsl/glsl_parser_extras.cpp
> >> index 046d5d7..d1cd8ff 100644
> >> --- a/src/glsl/glsl_parser_extras.cpp
> >> +++ b/src/glsl/glsl_parser_extras.cpp
> >> @@ -554,6 +554,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/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> >> index 9a0c24e..8572905 100644
> >> --- a/src/glsl/glsl_parser_extras.h
> >> +++ b/src/glsl/glsl_parser_extras.h
> >> @@ -378,6 +378,7 @@ struct _mesa_glsl_parse_state {
> >>
> >> /* ARB_viewport_array */
> >> unsigned MaxViewports;
> >> +
> >> } Const;
> >>
> >> /**
> >> @@ -430,6 +431,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/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> >> index 46f84c6..81c0dac 100644
> >> --- a/src/glsl/link_varyings.cpp
> >> +++ b/src/glsl/link_varyings.cpp
> >> @@ -404,7 +404,7 @@ tfeedback_decl::assign_location(struct gl_context *ctx,
> >> this->matched_candidate->type->fields.array->vector_elements;
> >> unsigned actual_array_size =
> >> (this->is_clip_distance_mesa || this->is_cull_distance_mesa) ?
> >> - prog->LastClipDistanceArraySize :
> >> + prog->LastClipCullDistanceArraySize :
> >> this->matched_candidate->type->array_size();
> >>
> >> if (this->is_subscripted) {
> >> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> >> index e616520..8bb1a5c 100644
> >> --- a/src/glsl/linker.cpp
> >> +++ b/src/glsl/linker.cpp
> >> @@ -474,49 +474,94 @@ link_invalidate_variable_locations(exec_list *ir)
> >> }
> >>
> >>
> >> +
> >> /**
> >> - * Set UsesClipDistance and ClipDistanceArraySize based on the given
> >> shader.
> >> + * Set UsesClipCullDistance and ClipCullDistanceArraySize based on the
> >> given
> >> + * shader.
> >> *
> >> - * Also check for errors based on incorrect usage of gl_ClipVertex and
> >> - * gl_ClipDistance.
> >> + * Also check for errors based on incorrect usage of gl_ClipVertex,
> >> + * 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, GLboolean *UsesClipDistance,
> >> - GLuint *ClipDistanceArraySize)
> >> +analyze_clip_cull_usage(struct gl_shader_program *prog,
> >> + struct gl_shader *shader,
> >> + struct gl_context *ctx,
> >> + GLboolean *UsesClipCullDistance,
> >> + GLuint *ClipCullDistanceArraySize)
> >> {
> >> - *ClipDistanceArraySize = 0;
> >> + *ClipCullDistanceArraySize = 0;
> >>
> >> - if (!prog->IsES && prog->Version >= 130) {
> >> - /* From section 7.1 (Vertex Shader Special Variables) of the
> >> - * GLSL 1.30 spec:
> >> - *
> >> - * "It is an error for a shader to statically write both
> >> - * gl_ClipVertex and gl_ClipDistance."
> >> - *
> >> - * This does not apply to GLSL ES shaders, since GLSL ES defines
> >> neither
> >> - * gl_ClipVertex nor gl_ClipDistance.
> >> - */
> >> + if (prog->IsES && prog->Version < 130) {
> >> + *UsesClipCullDistance = false;
> >> + } else {
> >> 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;
> >> }
> >> - *UsesClipDistance = clip_distance.variable_found();
> >> + 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;
> >> + }
> >> +
> >> + *UsesClipCullDistance = clip_distance.variable_found() |
> >> + cull_distance.variable_found();
> >> +
> >> ir_variable *clip_distance_var =
> >> shader->symbols->get_variable("gl_ClipDistance");
> >> + ir_variable *cull_distance_var =
> >> + shader->symbols->get_variable("gl_CullDistance");
> >> +
> >> if (clip_distance_var)
> >> - *ClipDistanceArraySize = clip_distance_var->type->length;
> >> - } else {
> >> - *UsesClipDistance = false;
> >> + *ClipCullDistanceArraySize =
> >> clip_distance_var->type->was_unsized ?
> >> + clip_distance_var->type->length -1 :
> >> + clip_distance_var->type->length;
> >> +
> >> + if (cull_distance_var)
> >> + *ClipCullDistanceArraySize+=
> >> cull_distance_var->type->was_unsized ?
> >> + cull_distance_var->type->length -1 :
> >> + cull_distance_var->type->length;
> >
> > How come this needs to be done for former unsized arrays?
> >
>
> We replace an unsized array with a default size of 1 ( fixup_type() ) ,
> so with either cull or clip set to max (8) and the other array left
> unsized we would end up with a total of 9 reported, that would trigger
> the error below.
I see. Is there a piglit test for this? I couldn't find it but I may be
blind. I take it the Nvidia driver passes in this case?
Wouldn't you only want the test below to pass if the unsized cull or
clip is unused? If it defaults to 1 (or is resized to 1) and its used
then isn't it valid for this to fail?
>
> >
> >> +
> >> + /* 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 (*ClipCullDistanceArraySize > 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);
> >> + }
> >> }
> >> }
> >>
> >> @@ -524,14 +569,14 @@ analyze_clip_usage(struct gl_shader_program *prog,
> >> /**
> >> * Verify that a vertex shader executable meets all semantic
> >> requirements.
> >> *
> >> - * Also sets prog->Vert.UsesClipDistance and
> >> prog->Vert.ClipDistanceArraySize
> >> - * as a side effect.
> >> + * Also sets prog->Vert.UsesClipCullDistance and
> >> + * prog->Vert.ClipCullDistanceArraySize 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;
> >> @@ -578,8 +623,8 @@ validate_vertex_shader_executable(struct
> >> gl_shader_program *prog,
> >> }
> >> }
> >>
> >> - analyze_clip_usage(prog, shader, &prog->Vert.UsesClipDistance,
> >> - &prog->Vert.ClipDistanceArraySize);
> >> + analyze_clip_cull_usage(prog, shader, ctx,
> >> &prog->Vert.UsesClipCullDistance,
> >> + &prog->Vert.ClipCullDistanceArraySize);
> >> }
> >>
> >>
> >> @@ -590,7 +635,7 @@ validate_vertex_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, struct gl_context
> >> *ctx)
> >> {
> >> if (shader == NULL)
> >> return;
> >> @@ -610,14 +655,14 @@ validate_fragment_shader_executable(struct
> >> gl_shader_program *prog,
> >> /**
> >> * Verify that a geometry shader executable meets all semantic
> >> requirements
> >> *
> >> - * Also sets prog->Geom.VerticesIn, prog->Geom.UsesClipDistance, and
> >> - * prog->Geom.ClipDistanceArraySize as a side effect.
> >> + * Also sets prog->Geom.VerticesIn, prog->Geom.UsesClipCullDistance, and
> >> + * prog->Geom.ClipCullDistanceArraySize 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;
> >> @@ -625,8 +670,8 @@ 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.UsesClipDistance,
> >> - &prog->Geom.ClipDistanceArraySize);
> >> + analyze_clip_cull_usage(prog, shader, ctx,
> >> &prog->Geom.UsesClipCullDistance,
> >> + &prog->Geom.ClipCullDistanceArraySize);
> >> }
> >>
> >> /**
> >> @@ -2834,13 +2879,13 @@ 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_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);
> >> + validate_fragment_shader_executable(prog, sh, ctx);
> >> break;
> >> }
> >> if (!prog->LinkStatus)
> >> @@ -2851,11 +2896,11 @@ link_shaders(struct gl_context *ctx, struct
> >> gl_shader_program *prog)
> >> }
> >>
> >> if (num_shaders[MESA_SHADER_GEOMETRY] > 0)
> >> - prog->LastClipDistanceArraySize = prog->Geom.ClipDistanceArraySize;
> >> + prog->LastClipCullDistanceArraySize =
> >> prog->Geom.ClipCullDistanceArraySize;
> >> else if (num_shaders[MESA_SHADER_VERTEX] > 0)
> >> - prog->LastClipDistanceArraySize = prog->Vert.ClipDistanceArraySize;
> >> + prog->LastClipCullDistanceArraySize =
> >> prog->Vert.ClipCullDistanceArraySize;
> >> else
> >> - prog->LastClipDistanceArraySize = 0; /* Not used */
> >> + prog->LastClipCullDistanceArraySize = 0; /* Not used */
> >>
> >> /* Here begins the inter-stage linking phase. Some initial
> >> validation is
> >> * performed, then locations are assigned for uniforms, attributes,
> >> and
> >> diff --git a/src/glsl/standalone_scaffolding.cpp
> >> b/src/glsl/standalone_scaffolding.cpp
> >> index a109c4e..0ac16bb 100644
> >> --- a/src/glsl/standalone_scaffolding.cpp
> >> +++ b/src/glsl/standalone_scaffolding.cpp
> >> @@ -142,6 +142,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/glsl/tests/varyings_test.cpp
> >> b/src/glsl/tests/varyings_test.cpp
> >> index 4573529..7a962c5 100644
> >> --- a/src/glsl/tests/varyings_test.cpp
> >> +++ b/src/glsl/tests/varyings_test.cpp
> >> @@ -202,6 +202,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 =
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
