Re: [Mesa-dev] [PATCH 05/30] glsl/linker: Refactor in preparation for adding more shader stages.
On 9 January 2014 22:17, Chris Forbes wrote: > 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. > Yeah, that's a good point. I agree that it's completely benign, but I think I could have made things clearer. I've changed it to this: if (num_shaders[MESA_SHADER_GEOMETRY] > 0) prog->LastClipDistanceArraySize = prog->Geom.ClipDistanceArraySize; else if (num_shaders[MESA_SHADER_VERTEX] > 0) prog->LastClipDistanceArraySize = prog->Vert.ClipDistanceArraySize; else prog->LastClipDistanceArraySize = 0; /* Not used */ ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/30] glsl/linker: Refactor in preparation for adding more shader stages.
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
On Fri, Jan 10, 2014 at 3:19 PM, Paul Berry 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->Li
[Mesa-dev] [PATCH 05/30] glsl/linker: Refactor in preparation for adding more shader stages.
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,
-
