Sorry I missed the first round of feedback on these patches. I hope my comments aren't coming too late. On 1 November 2011 12:48, Marek Olšák <<a href="mailto:maraeo@gmail.com" target="_blank">maraeo@gmail.com</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> From: Dan McCabe <<a href="mailto:zen3d.linux@gmail.com" target="_blank">zen3d.linux@gmail.com</a>> Modify the linker to assign additional slots for varying variables used by transform feedback. This is done after other varyings are already assigned slots. Since this is done after previous varying slot assignments, the code needs to know how many varyings are already assigned slots. A new function "max_varying()" is introduced to examine a previously processed shader to find the largest varying already assigned a slot. All new varyings will be assigned slots after this one. If varyings are found, -1 is returned. [ Marek Olšák: fix segfault in accessing names ] --- src/glsl/linker.cpp | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 84 insertions(+), 0 deletions(-) diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp index beadec6..e6012bf 100644 --- a/src/glsl/linker.cpp +++ b/src/glsl/linker.cpp @@ -207,6 +207,25 @@ invalidate_variable_locations(gl_shader *sh, enum ir_variable_mode mode, } } + +int +max_varying(gl_shader *sh, enum ir_variable_mode mode) +{ + int max_varying = -1; + + foreach_list(node, sh->ir) { + ir_variable *const var = ((ir_instruction *) node)->as_variable(); + + if ((var == NULL) || (var->mode != (unsigned) mode)) + continue; + + if (var->location > max_varying) + max_varying = var->location; + } + + return max_varying; +} </blockquote><div> A few comments on this function: 1. It should be declared static, since it's only used in this file. 2. Since it's only called with mode == ir_var_out, it might make more sense to drop that argument and call the function max_out_location(). 3. Judging by how the return value of this function is used (it has 1 added to it and then the value is adjusted to be at least VERT_RESULT_VAR0), it seems like the real purpose of this function is to figure out the first slot into which varyings can be safely assigned. Consider renaming this function to something like "find_free_varying_slots" and having it return the first slot into which varyings can be safely assigned. 4. This function might be unnecessary--see my comment below. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex"> + /** * Determine the number of attribute slots required for a particular type @@ -1597,6 +1616,69 @@ assign_varying_locations(struct gl_context *ctx, void +assign_transform_feedback_varying_locations(struct gl_context *ctx, + struct gl_shader_program *prog) </blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex"> +{ + struct gl_shader *vs = prog->_LinkedShaders[MESA_SHADER_VERTEX]; + + if (vs == NULL) + return; + + char **names = prog->TransformFeedback.VaryingNames; + int num_names = prog->TransformFeedback.NumVarying; + + if (num_names <= 0) + return; + + int num_varying = max_varying(vs, ir_var_out) + 1; + unsigned output_index = + (num_varying > VERT_RESULT_VAR0) + ? num_varying + : VERT_RESULT_VAR0; </blockquote><div> The effect of this code is to just recompute the value that output_index had when assign_varying_locations() finished. Wouldn't it be better to just have assign_varying_locations() just store the value of output_index somewhere so we can retrieve it here? Or, perhaps it would be better to merge all of this code into assign_varying_locations(). Really, both of these functions are doing the same task, they are just using different criteria (assign_varying_locations assigns locations to vertex shader outputs if they are used by the fragment shader, this function assigns locations to vertex shader outputs if they are used by transform feedback). </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> + + foreach_list(node, vs->ir) { + ir_variable *const output_var = ((ir_instruction *) node)->as_variable(); + + if (output_var == NULL + || output_var->mode != ir_var_out + || output_var->location != -1) + continue; + + /* Find a transform feedback varying variable that has + * the same name as the shader variable. + */ + int varying_index = -1; + for (int num_name = 0; num_name < num_names; num_name++) { + char *name = names[num_name]; + if (strcmp(output_var->name, name) == 0) { + varying_index = num_name; + break; + } + } + + if (varying_index == -1) + continue; + + output_var->location = output_index; + + /* FINISHME: Support for "varying" records in GLSL 1.50. */ + assert(!output_var->type->is_record()); + + if (output_var->type->is_array()) { + const unsigned slots = output_var->type->length + * output_var->type->fields.array->matrix_columns; + + output_index += slots; + } else { + const unsigned slots = output_var->type->matrix_columns; + + output_index += slots; + } </blockquote><div> Aside: I have just realized that the technique we used to implement the gl_ClipDistance[] array (to lower it from an array of 8 floats to an array of 2 vec4s) is going to throw a monkeywrench into transform feedback of gl_ClipDistance. I'll make sure to add that to the transform feedback tests I'm writing. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex"> + } +} </blockquote><div> I don't see any error checking to make sure that we don't exceed the value of ctx->Const.MaxVarying. (According to the EXT_transform_feedback spec, "Varying variables specified by TransformFeedbackVaryingsEXT are always considered active and count against the total limit on the number of active varying components, regardless of the needs of the fragment shader."). Normally this check would be done in assign_varying_locations(), but that function finishes before this one is called. This is another argument, IMHO, for merging the two functions together. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex"> + + +void link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) { void *mem_ctx = ralloc_context(NULL); // temporary linker context @@ -1778,6 +1860,8 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) prev = i; } + assign_transform_feedback_varying_locations(ctx, prog); + if (prog->_LinkedShaders[MESA_SHADER_VERTEX] != NULL) { demote_shader_inputs_and_outputs(prog->_LinkedShaders[MESA_SHADER_VERTEX], ir_var_out); -- 1.7.4.1 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>