[Mesa-dev] [PATCH] draw: make sure key size is calculated consistently.

[Roland Scheidegger]

Am 19.02.2013 15:57, schrieb Jose Fonseca:
> There may be more vertex elements that used in the shader. But why should the key contain those elements? Won't this cause needless recompilations (e.g., in situations where the state tracker leaves unneeded elements from previous draw?)?
I don't think the state tracker would leave unneeded elements like that
(that is I think the nr_elements would be adjusted if the state tracker
has to figure it out on its own, causing recompiles in any case).
But yes if you set different pipe_vertex_element which only differ in
the unused elements then it will cause unnecessary recompile (I don't
think that's really something which matters here).

> 
> That is, it seems to be that the key should have the number of elements from pipe_vertex_element information, but only copy those that vertex shader uses.
That doesn't sound very good. If we want to dump the
pipe_vertex_elements like we do now either we need to fix up the
nr_elements or copy all of them. Also vs_generate function seems to
create code for all pipe_vertex_elements, not just those used by the shader.
I guess that instead of using nr_elements we could just use the
information from the shader instead consistently, though I'm actually
unsure this works always - is it somehow possible to only use
vertex_element nr 2 and 4 for instance?

So I think you're suggesting instead of this fix that key->nr_elements
wouldn't be used for anything except the key comparison itself, and
everything else (calculating sampler offset in the key, tgsi dump, code
generation) would use the shader information?

Roland

> 
> Jose
> 
> 
> ----- Original Message -----
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> Some parts calculated key size by using shader information, others by using
>> the pipe_vertex_element information. Since it is perfectly valid to have more
>> vertex_elements set than the vertex shader is using those may not be the
>> same,
>> so we weren't copying over all vertex_element state - this caused the tgsi
>> dump
>> to assert (iterates over all vertex elements). With some luck it didn't
>> crash otherwise even though the llvm generate_fetch code also iterates over
>> all vertex elements (probably because llvm threw away the unused inputs
>> anyway),
>> but if in this situation vertex texturing would be used things would
>> definitely
>> go wrong (as the sampler information wouldn't be copied).
>> So drop the key size calculation using shader information.
>> ---
>>  src/gallium/auxiliary/draw/draw_llvm.c             |   13 ++++++++-----
>>  src/gallium/auxiliary/draw/draw_llvm.h             |    1 -
>>  .../draw/draw_pt_fetch_shade_pipeline_llvm.c       |    7 ++++++-
>>  src/gallium/auxiliary/draw/draw_vs_llvm.c          |    6 ------
>>  4 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c
>> b/src/gallium/auxiliary/draw/draw_llvm.c
>> index f3bbbbb..df57358 100644
>> --- a/src/gallium/auxiliary/draw/draw_llvm.c
>> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
>> @@ -420,17 +420,20 @@ draw_llvm_destroy(struct draw_llvm *llvm)
>>   */
>>  struct draw_llvm_variant *
>>  draw_llvm_create_variant(struct draw_llvm *llvm,
>> -			 unsigned num_inputs,
>> -			 const struct draw_llvm_variant_key *key)
>> +                         unsigned num_inputs,
>> +                         const struct draw_llvm_variant_key *key)
>>  {
>>     struct draw_llvm_variant *variant;
>>     struct llvm_vertex_shader *shader =
>>        llvm_vertex_shader(llvm->draw->vs.vertex_shader);
>>     LLVMTypeRef vertex_header;
>> +   unsigned key_size = draw_llvm_variant_key_size(key->nr_vertex_elements,
>> +                                                  MAX2(key->nr_samplers,
>> +
>> key->nr_sampler_views));
>>  
>>     variant = MALLOC(sizeof *variant +
>> -		    shader->variant_key_size -
>> -		    sizeof variant->key);
>> +                    key_size -
>> +                    sizeof variant->key);
>>     if (variant == NULL)
>>        return NULL;
>>  
>> @@ -440,7 +443,7 @@ draw_llvm_create_variant(struct draw_llvm *llvm,
>>  
>>     create_jit_types(variant);
>>  
>> -   memcpy(&variant->key, key, shader->variant_key_size);
>> +   memcpy(&variant->key, key, key_size);
>>  
>>     vertex_header = create_jit_vertex_header(variant->gallivm, num_inputs);
>>  
>> diff --git a/src/gallium/auxiliary/draw/draw_llvm.h
>> b/src/gallium/auxiliary/draw/draw_llvm.h
>> index 17ca304..b20cee5 100644
>> --- a/src/gallium/auxiliary/draw/draw_llvm.h
>> +++ b/src/gallium/auxiliary/draw/draw_llvm.h
>> @@ -281,7 +281,6 @@ struct draw_llvm_variant
>>  struct llvm_vertex_shader {
>>     struct draw_vertex_shader base;
>>  
>> -   unsigned variant_key_size;
>>     struct draw_llvm_variant_list_item variants;
>>     unsigned variants_created;
>>     unsigned variants_cached;
>> diff --git a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
>> b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
>> index b0c18ed..d7f855f 100644
>> --- a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
>> +++ b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
>> @@ -127,13 +127,18 @@ llvm_middle_end_prepare( struct draw_pt_middle_end
>> *middle,
>>        struct llvm_vertex_shader *shader = llvm_vertex_shader(vs);
>>        char store[DRAW_LLVM_MAX_VARIANT_KEY_SIZE];
>>        unsigned i;
>> +      unsigned key_size;
>>  
>>        key = draw_llvm_make_variant_key(fpme->llvm, store);
>>  
>> +      key_size = draw_llvm_variant_key_size(key->nr_vertex_elements,
>> +                                            MAX2(key->nr_samplers,
>> +                                                 key->nr_sampler_views));
>> +
>>        /* Search shader's list of variants for the key */
>>        li = first_elem(&shader->variants);
>>        while (!at_end(&shader->variants, li)) {
>> -         if (memcmp(&li->base->key, key, shader->variant_key_size) == 0) {
>> +         if (memcmp(&li->base->key, key, key_size) == 0) {
>>              variant = li->base;
>>              break;
>>           }
>> diff --git a/src/gallium/auxiliary/draw/draw_vs_llvm.c
>> b/src/gallium/auxiliary/draw/draw_vs_llvm.c
>> index ac3999e..50cef79 100644
>> --- a/src/gallium/auxiliary/draw/draw_vs_llvm.c
>> +++ b/src/gallium/auxiliary/draw/draw_vs_llvm.c
>> @@ -98,12 +98,6 @@ draw_create_vs_llvm(struct draw_context *draw,
>>  
>>     tgsi_scan_shader(state->tokens, &vs->base.info);
>>  
>> -   vs->variant_key_size =
>> -      draw_llvm_variant_key_size(
>> -         vs->base.info.file_max[TGSI_FILE_INPUT]+1,
>> -         MAX2(vs->base.info.file_max[TGSI_FILE_SAMPLER]+1,
>> -              vs->base.info.file_max[TGSI_FILE_SAMPLER_VIEW]+1));
>> -
>>     vs->base.state.stream_output = state->stream_output;
>>     vs->base.draw = draw;
>>     vs->base.prepare = vs_llvm_prepare;
>> --
>> 1.7.9.5
>>