On 06/16/2016 12:12 PM, Ilia Mirkin wrote: > On Thu, Jun 16, 2016 at 3:06 PM, Ian Romanick <i...@freedesktop.org> wrote: >> From: Ian Romanick <ian.d.roman...@intel.com> >> >> Outputs from the vertex shader need to be able to match >> per-vertex-arrayed inputs of later stages. Acomplish this by stripping >> one level of arrayness from the names and types of outputs going to a >> per-vertex-arrayed stage. >> >> v2: Add missing checks for TESS_EVAL->GEOMETRY. Noticed by Timothy >> Arceri. >> >> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96358 >> Cc: "12.0" <mesa-sta...@lists.freedesktop.org> >> Cc: Gregory Hainaut <gregory.hain...@gmail.com> >> Cc: Ilia Mirkin <imir...@alum.mit.edu> >> --- >> src/mesa/main/shader_query.cpp | 100 >> +++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 92 insertions(+), 8 deletions(-) >> >> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp >> index 5956ce4..a6a0a2f 100644 >> --- a/src/mesa/main/shader_query.cpp >> +++ b/src/mesa/main/shader_query.cpp >> @@ -1385,13 +1385,26 @@ _mesa_get_program_resourceiv(struct >> gl_shader_program *shProg, >> >> static bool >> validate_io(struct gl_shader_program *producer, >> - struct gl_shader_program *consumer) >> + struct gl_shader_program *consumer, >> + gl_shader_stage producer_stage, >> + gl_shader_stage consumer_stage) >> { >> if (producer == consumer) >> return true; >> >> + const bool nonarray_stage_to_array_stage = >> + (producer_stage == MESA_SHADER_VERTEX && >> + (consumer_stage == MESA_SHADER_GEOMETRY || >> + consumer_stage == MESA_SHADER_TESS_CTRL || >> + consumer_stage == MESA_SHADER_TESS_EVAL)) || >> + (producer_stage == MESA_SHADER_TESS_EVAL && >> + consumer_stage == MESA_SHADER_GEOMETRY); > > Since TCS -> GEOM is not possible, isn't the only way that array -> > array can happen is TCS -> TES? IOW, couldn't this just be > > producer != TCS && (consumer == TCS || consumer == TES || consumer == GS) > > Your call whether you want to rewrite it. [No comment on the rest of > the patch...]
I had though of another (slightly less) clever way to do this, but I decided against it because it was less clear. Anyway, I tried your suggestion. Here's the assembly 'diff --side-by-side'. Mine is on the left, yours is on the right. mov 0x4(%rax),%eax mov 0x4(%rax),%eax cmp %rbp,%rbx cmp %rbp,%rbx je 374c | je 3744 test %eax,%eax | movb $0x0,0x1f(%rsp) jne 357b | cmp $0x1,%eax lea -0x1(%rdx),%ecx | je 357f movb $0x1,0x1f(%rsp) | sub $0x1,%edx cmp $0x2,%ecx | cmp $0x2,%edx jbe 358d | setbe 0x1f(%rsp) cmp $0x2,%eax < sete %cl < cmp $0x3,%edx < sete %al < and %eax,%ecx < mov %cl,0x1f(%rsp) < I don't know that we'd be able to measure a performance difference. Maybe having one less branch that may not predict well is good? *shrug* > -ilia > >> + >> bool valid = true; >> >> + void *name_buffer = NULL; >> + size_t name_buffer_size = 0; >> + >> gl_shader_variable const **outputs = >> (gl_shader_variable const **) calloc(producer->NumProgramResourceList, >> sizeof(gl_shader_variable *)); >> @@ -1463,11 +1476,52 @@ validate_io(struct gl_shader_program *producer, >> } >> } >> } else { >> + char *consumer_name = consumer_var->name; >> + >> + if (nonarray_stage_to_array_stage && >> + consumer_var->interface_type != NULL && >> + consumer_var->interface_type->is_array() && >> + !is_gl_identifier(consumer_var->name)) { >> + const size_t name_len = strlen(consumer_var->name); >> + >> + if (name_len >= name_buffer_size) { >> + free(name_buffer); >> + >> + name_buffer_size = name_len + 1; >> + name_buffer = malloc(name_buffer_size); >> + if (name_buffer == NULL) { >> + valid = false; >> + goto out; >> + } >> + } >> + >> + consumer_name = (char *) name_buffer; >> + >> + char *s = strchr(consumer_var->name, '['); >> + if (s == NULL) { >> + valid = false; >> + goto out; >> + } >> + >> + char *t = strchr(s, ']'); >> + if (t == NULL) { >> + valid = false; >> + goto out; >> + } >> + >> + assert(t[1] == '.' || t[1] == '['); >> + >> + const ptrdiff_t base_name_len = s - consumer_var->name; >> + >> + memcpy(consumer_name, consumer_var->name, base_name_len); >> + strcpy(consumer_name + base_name_len, t + 1); >> + } >> + >> for (unsigned j = 0; j < num_outputs; j++) { >> const gl_shader_variable *const var = outputs[j]; >> >> if (!var->explicit_location && >> - strcmp(consumer_var->name, var->name) == 0) { >> + strcmp(consumer_name, var->name) == 0) { >> producer_var = var; >> match_index = j; >> break; >> @@ -1529,25 +1583,53 @@ validate_io(struct gl_shader_program *producer, >> * Note that location mismatches are detected by the loops above that >> * find the producer variable that goes with the consumer variable. >> */ >> - if (producer_var->type != consumer_var->type || >> - producer_var->interpolation != consumer_var->interpolation || >> - producer_var->precision != consumer_var->precision) { >> + if (nonarray_stage_to_array_stage) { >> + if (!consumer_var->type->is_array() || >> + consumer_var->type->fields.array != producer_var->type) { >> + valid = false; >> + goto out; >> + } >> + >> + if (consumer_var->interface_type != NULL) { >> + if (!consumer_var->interface_type->is_array() || >> + consumer_var->interface_type->fields.array != >> producer_var->interface_type) { >> + valid = false; >> + goto out; >> + } >> + } else if (producer_var->interface_type != NULL) { >> + valid = false; >> + goto out; >> + } >> + } else { >> + if (producer_var->type != consumer_var->type) { >> + valid = false; >> + goto out; >> + } >> + >> + if (producer_var->interface_type != consumer_var->interface_type) { >> + valid = false; >> + goto out; >> + } >> + } >> + >> + if (producer_var->interpolation != consumer_var->interpolation) { >> valid = false; >> goto out; >> } >> >> - if (producer_var->outermost_struct_type != >> consumer_var->outermost_struct_type) { >> + if (producer_var->precision != consumer_var->precision) { >> valid = false; >> goto out; >> } >> >> - if (producer_var->interface_type != consumer_var->interface_type) { >> + if (producer_var->outermost_struct_type != >> consumer_var->outermost_struct_type) { >> valid = false; >> goto out; >> } >> } >> >> out: >> + free(name_buffer); >> free(outputs); >> return valid && num_outputs == 0; >> } >> @@ -1579,7 +1661,9 @@ _mesa_validate_pipeline_io(struct gl_pipeline_object >> *pipeline) >> if (shProg[idx]->_LinkedShaders[idx]->Stage == MESA_SHADER_COMPUTE) >> break; >> >> - if (!validate_io(shProg[prev], shProg[idx])) >> + if (!validate_io(shProg[prev], shProg[idx], >> + shProg[prev]->_LinkedShaders[prev]->Stage, >> + shProg[idx]->_LinkedShaders[idx]->Stage)) >> return false; >> >> prev = idx; >> -- >> 2.5.5 >> > _______________________________________________ > mesa-stable mailing list > mesa-sta...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-stable > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev