[Mesa-dev] [PATCH 01/30] glsl: Refactor handling of gl_ClipDistance/gl_ClipVertex linkage rules for GS.
Kenneth Graunke
kenneth at whitecape.org
Tue Aug 20 11:48:48 PDT 2013
On 08/20/2013 11:30 AM, Paul Berry wrote:
> This patch extracts the following logic from
> validate_vertex_shader_executable():
>
> (a) Generate an error if the shader writes to both gl_ClipDistance and
> gl_ClipVertex.
>
> (b) Record whether the shader writes to gl_ClipDistance in
> gl_shader_program for use by the back-end.
>
> (c) Record the size of gl_ClipDistance in gl_shader_program for use by
> transform feedback logic.
>
> And moves it into a function that is shared between vertex and
> geometry shaders.
>
> Strictly speaking we only need to have shared logic for (b) and (c)
> right now (since (a) only matters in compatibility contexts, and we're
> only implementing geometry shaders in core contexts right now). But
> the three are closely related enough that it seems sensible to keep
> them together.
> ---
> src/glsl/linker.cpp | 82 ++++++++++++++++++++++++++++++-----------------
> src/mesa/main/mtypes.h | 8 +++++
> src/mesa/main/shaderapi.c | 1 +
> 3 files changed, 62 insertions(+), 29 deletions(-)
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index f87ae0e..8430096 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -373,6 +373,52 @@ link_invalidate_variable_locations(gl_shader *sh, int input_base,
>
>
> /**
> + * Set UsesClipDistance and ClipDistanceArraySize based on the given shader.
> + *
> + * Also check for errors based on incorrect usage of gl_ClipVertex and
> + * gl_ClipDistance.
> + *
> + * Return false if an error was reported.
> + */
> +static void
> +analyze_clip_usage(const char *shader_type, struct gl_shader_program *prog,
> + struct gl_shader *shader, GLboolean *UsesClipDistance,
> + GLuint *ClipDistanceArraySize)
Maybe it's just me, but I'm always a bit weirded out when I see C++ code
using pass-by-pointer for out parameters, rather than using
pass-by-reference.
But perhaps that's easier for the C programmers here. *shrug*
> +{
> + *ClipDistanceArraySize = 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.
> + */
> + find_assignment_visitor clip_vertex("gl_ClipVertex");
> + find_assignment_visitor clip_distance("gl_ClipDistance");
> +
> + clip_vertex.run(shader->ir);
> + clip_distance.run(shader->ir);
> + if (clip_vertex.variable_found() && clip_distance.variable_found()) {
> + linker_error(prog, "%s shader writes to both `gl_ClipVertex' "
> + "and `gl_ClipDistance'\n", shader_type);
> + return;
> + }
> + *UsesClipDistance = clip_distance.variable_found();
> + ir_variable *clip_distance_var =
> + shader->symbols->get_variable("gl_ClipDistance");
> + if (clip_distance_var)
> + *ClipDistanceArraySize = clip_distance_var->type->length;
> + } else {
> + *UsesClipDistance = false;
> + }
> +}
> +
> +
> +/**
> * Verify that a vertex shader executable meets all semantic requirements.
> *
> * Also sets prog->Vert.UsesClipDistance and prog->Vert.ClipDistanceArraySize
> @@ -422,34 +468,8 @@ validate_vertex_shader_executable(struct gl_shader_program *prog,
> }
> }
>
> - prog->Vert.ClipDistanceArraySize = 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.
> - */
> - find_assignment_visitor clip_vertex("gl_ClipVertex");
> - find_assignment_visitor clip_distance("gl_ClipDistance");
> -
> - clip_vertex.run(shader->ir);
> - clip_distance.run(shader->ir);
> - if (clip_vertex.variable_found() && clip_distance.variable_found()) {
> - linker_error(prog, "vertex shader writes to both `gl_ClipVertex' "
> - "and `gl_ClipDistance'\n");
> - return;
> - }
> - prog->Vert.UsesClipDistance = clip_distance.variable_found();
> - ir_variable *clip_distance_var =
> - shader->symbols->get_variable("gl_ClipDistance");
> - if (clip_distance_var)
> - prog->Vert.ClipDistanceArraySize = clip_distance_var->type->length;
> - }
> + analyze_clip_usage("vertex", prog, shader, &prog->Vert.UsesClipDistance,
> + &prog->Vert.ClipDistanceArraySize);
> }
>
>
> @@ -480,7 +500,8 @@ validate_fragment_shader_executable(struct gl_shader_program *prog,
> /**
> * Verify that a geometry shader executable meets all semantic requirements
> *
> - * Also sets prog->Geom.VerticesIn as a side effect.
> + * Also sets prog->Geom.VerticesIn, prog->Geom.UsesClipDistance, and
> + * prog->Geom.ClipDistanceArraySize as a side effect.
> *
> * \param shader Geometry shader executable to be verified
> */
> @@ -493,6 +514,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("geometry", prog, shader, &prog->Geom.UsesClipDistance,
> + &prog->Geom.ClipDistanceArraySize);
> }
>
>
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 5f9b7f9..22bb58c 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -1924,6 +1924,7 @@ struct gl_geometry_program
> GLenum InputType; /**< GL_POINTS, GL_LINES, GL_LINES_ADJACENCY_ARB,
> GL_TRIANGLES, or GL_TRIANGLES_ADJACENCY_ARB */
> GLenum OutputType; /**< GL_POINTS, GL_LINE_STRIP or GL_TRIANGLE_STRIP */
> + GLboolean UsesClipDistance;
> };
>
>
> @@ -2348,6 +2349,13 @@ struct gl_shader_program
> GLenum InputType; /**< GL_POINTS, GL_LINES, GL_LINES_ADJACENCY_ARB,
> GL_TRIANGLES, or GL_TRIANGLES_ADJACENCY_ARB */
> GLenum OutputType; /**< GL_POINTS, GL_LINE_STRIP or GL_TRIANGLE_STRIP */
> + /**
> + * True if gl_ClipDistance is written to. Copied into
> + * gl_geometry_program by _mesa_copy_linked_program_data().
> + */
> + GLboolean UsesClipDistance;
> + GLuint ClipDistanceArraySize; /**< Size of the gl_ClipDistance array, or
> + 0 if not present. */
I don't think these inline comments are supposed to span multiple lines.
I'd prefer:
/** Size of the gl_ClipDistance array, or 0 if not present. */
GLuint ClipDistanceArraySize;
Regardless of these trivial nits,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> } Geom;
>
> /** Vertex shader state */
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index d184b11..4fe9d9c 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -1871,6 +1871,7 @@ _mesa_copy_linked_program_data(gl_shader_type type,
> dst_gp->VerticesOut = src->Geom.VerticesOut;
> dst_gp->InputType = src->Geom.InputType;
> dst_gp->OutputType = src->Geom.OutputType;
> + dst_gp->UsesClipDistance = src->Geom.UsesClipDistance;
> }
> break;
> default:
>
More information about the mesa-dev
mailing list