[Mesa-dev] [PATCH 05/30] glsl/linker: Refactor in preparation for adding more shader stages.
Chris Forbes
chrisf at ijw.co.nz
Thu Jan 9 22:17:08 PST 2014
This is a nice cleanup; I like that this brings both writes to
prog->LastClipDistanceArraySize together -- but it looks like the
behavior changes slightly.
Previously, if there was no VS and no GS, then we would never write
prog->LastClipDistanceArraySize. Now we'll read an old junk value
(potentially from a previous linking of the same program object with
different shaders attached) from prog->Vert.ClipDistanceArraySize
(since we never called validate_vertex_shader_executable) -- but we'll
never end up actually using it, since it's only used for transform
feedback of gl_ClipDistance.
If this is indeed how you intended it to work, and agree that it's
completely benign, then patches 1-12 are:
Reviewed-by: Chris Forbes <chrisf at ijw.co.nz>
On Fri, Jan 10, 2014 at 3:19 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> Rather than maintain separately named arrays and counts for vertex,
> geometry, and fragment shaders, just maintain these as arrays indexed
> by the gl_shader_type enum.
> ---
> src/glsl/linker.cpp | 114 ++++++++++++++++++----------------------------------
> 1 file changed, 39 insertions(+), 75 deletions(-)
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index e820f0f..f3fd66f 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -1994,19 +1994,14 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>
> /* Separate the shaders into groups based on their type.
> */
> - struct gl_shader **vert_shader_list;
> - unsigned num_vert_shaders = 0;
> - struct gl_shader **frag_shader_list;
> - unsigned num_frag_shaders = 0;
> - struct gl_shader **geom_shader_list;
> - unsigned num_geom_shaders = 0;
> -
> - vert_shader_list = (struct gl_shader **)
> - calloc(prog->NumShaders, sizeof(struct gl_shader *));
> - frag_shader_list = (struct gl_shader **)
> - calloc(prog->NumShaders, sizeof(struct gl_shader *));
> - geom_shader_list = (struct gl_shader **)
> - calloc(prog->NumShaders, sizeof(struct gl_shader *));
> + struct gl_shader **shader_list[MESA_SHADER_STAGES];
> + unsigned num_shaders[MESA_SHADER_STAGES];
> +
> + for (int i = 0; i < MESA_SHADER_STAGES; i++) {
> + shader_list[i] = (struct gl_shader **)
> + calloc(prog->NumShaders, sizeof(struct gl_shader *));
> + num_shaders[i] = 0;
> + }
>
> unsigned min_version = UINT_MAX;
> unsigned max_version = 0;
> @@ -2022,20 +2017,9 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> goto done;
> }
>
> - switch (prog->Shaders[i]->Stage) {
> - case MESA_SHADER_VERTEX:
> - vert_shader_list[num_vert_shaders] = prog->Shaders[i];
> - num_vert_shaders++;
> - break;
> - case MESA_SHADER_FRAGMENT:
> - frag_shader_list[num_frag_shaders] = prog->Shaders[i];
> - num_frag_shaders++;
> - break;
> - case MESA_SHADER_GEOMETRY:
> - geom_shader_list[num_geom_shaders] = prog->Shaders[i];
> - num_geom_shaders++;
> - break;
> - }
> + gl_shader_stage shader_type = prog->Shaders[i]->Stage;
> + shader_list[shader_type][num_shaders[shader_type]] = prog->Shaders[i];
> + num_shaders[shader_type]++;
> }
>
> /* In desktop GLSL, different shader versions may be linked together. In
> @@ -2052,7 +2036,8 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>
> /* Geometry shaders have to be linked with vertex shaders.
> */
> - if (num_geom_shaders > 0 && num_vert_shaders == 0) {
> + if (num_shaders[MESA_SHADER_GEOMETRY] > 0 &&
> + num_shaders[MESA_SHADER_VERTEX] == 0) {
> linker_error(prog, "Geometry shader must be linked with "
> "vertex shader\n");
> goto done;
> @@ -2067,55 +2052,37 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>
> /* Link all shaders for a particular stage and validate the result.
> */
> - if (num_vert_shaders > 0) {
> - gl_shader *const sh =
> - link_intrastage_shaders(mem_ctx, ctx, prog, vert_shader_list,
> - num_vert_shaders);
> -
> - if (!prog->LinkStatus)
> - goto done;
> -
> - validate_vertex_shader_executable(prog, sh);
> - if (!prog->LinkStatus)
> - goto done;
> - prog->LastClipDistanceArraySize = prog->Vert.ClipDistanceArraySize;
> + for (int stage = 0; stage < MESA_SHADER_STAGES; stage++) {
> + if (num_shaders[stage] > 0) {
> + gl_shader *const sh =
> + link_intrastage_shaders(mem_ctx, ctx, prog, shader_list[stage],
> + num_shaders[stage]);
>
> - _mesa_reference_shader(ctx, &prog->_LinkedShaders[MESA_SHADER_VERTEX],
> - sh);
> - }
> -
> - if (num_frag_shaders > 0) {
> - gl_shader *const sh =
> - link_intrastage_shaders(mem_ctx, ctx, prog, frag_shader_list,
> - num_frag_shaders);
> -
> - if (!prog->LinkStatus)
> - goto done;
> + if (!prog->LinkStatus)
> + goto done;
>
> - validate_fragment_shader_executable(prog, sh);
> - if (!prog->LinkStatus)
> - goto done;
> + switch (stage) {
> + case MESA_SHADER_VERTEX:
> + validate_vertex_shader_executable(prog, sh);
> + break;
> + case MESA_SHADER_GEOMETRY:
> + validate_geometry_shader_executable(prog, sh);
> + break;
> + case MESA_SHADER_FRAGMENT:
> + validate_fragment_shader_executable(prog, sh);
> + break;
> + }
> + if (!prog->LinkStatus)
> + goto done;
>
> - _mesa_reference_shader(ctx, &prog->_LinkedShaders[MESA_SHADER_FRAGMENT],
> - sh);
> + _mesa_reference_shader(ctx, &prog->_LinkedShaders[stage], sh);
> + }
> }
>
> - if (num_geom_shaders > 0) {
> - gl_shader *const sh =
> - link_intrastage_shaders(mem_ctx, ctx, prog, geom_shader_list,
> - num_geom_shaders);
> -
> - if (!prog->LinkStatus)
> - goto done;
> -
> - validate_geometry_shader_executable(prog, sh);
> - if (!prog->LinkStatus)
> - goto done;
> + if (num_shaders[MESA_SHADER_GEOMETRY] > 0)
> prog->LastClipDistanceArraySize = prog->Geom.ClipDistanceArraySize;
> -
> - _mesa_reference_shader(ctx, &prog->_LinkedShaders[MESA_SHADER_GEOMETRY],
> - sh);
> - }
> + else
> + prog->LastClipDistanceArraySize = prog->Vert.ClipDistanceArraySize;
>
> /* Here begins the inter-stage linking phase. Some initial validation is
> * performed, then locations are assigned for uniforms, attributes, and
> @@ -2373,11 +2340,8 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> /* FINISHME: Assign fragment shader output locations. */
>
> done:
> - free(vert_shader_list);
> - free(frag_shader_list);
> - free(geom_shader_list);
> -
> for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
> + free(shader_list[i]);
> if (prog->_LinkedShaders[i] == NULL)
> continue;
>
> --
> 1.8.5.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list