[Mesa-dev] [PATCH v2] glsl: move uniform calculation to link_uniforms

[Tapani Pälli]

On 01/20/2016 06:37 PM, Ilia Mirkin wrote:
> On Wed, Jan 20, 2016 at 6:35 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
>> On 01/20/2016 01:11 PM, Ilia Mirkin wrote:
>>>
>>> On Wed, Jan 20, 2016 at 6:02 AM, Tapani Pälli <tapani.palli at intel.com>
>>> wrote:
>>>>>
>>>>> Unfortunately putting such a shader together is a bit of a pain, since
>>>>> all the uniforms have to be used. I still really think you need to
>>>>> build a heap. Or at least store a "first empty slot" so that you don't
>>>>> repeat the iteration *every* time.
>>>>
>>>>
>>>> Yeah, I agree "first empty slot" would be good addition and will already
>>>> help a lot in problematic cases.
>>>
>>> Actually it's still not so great, since you can have stupid situations
>>> which won't fit into the first empty position. e.g. one empty
>>> location, and a bunch of foo[2] things. That's why I like using a heap
>>> :)
>
> With this shader: http://hastebin.com/sisomonenu.avrasm and this patch:
>
> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> index 76ee70d..a826e63 100644
> --- a/src/glsl/link_uniforms.cpp
> +++ b/src/glsl/link_uniforms.cpp
> @@ -1066,10 +1066,12 @@ find_empty_block(struct gl_shader_program *prog,
>                    struct gl_uniform_storage *uniform)
>   {
>      const unsigned entries = MAX2(1, uniform->array_elements);
> +static int loops = 0;
>      for (unsigned i = 0, j; i < prog->NumUniformRemapTable; i++) {
>         /* We found empty space in UniformRemapTable. */
>         if (prog->UniformRemapTable[i] == NULL) {
> -         for (j = i; j < entries && j < prog->NumUniformRemapTable; j++) {
> +         for (j = i; j - i < entries && j < prog->NumUniformRemapTable; j++) {
> +loops++;
>               if (prog->UniformRemapTable[j] != NULL) {
>                  /* Entries do not fit in this space, continue searching
>                   * after this location.
> @@ -1080,10 +1082,12 @@ find_empty_block(struct gl_shader_program *prog,
>            }
>            /* Entries fit, we can return this location. */
>            if (i != j + 1) {
> +printf("loops: %d\n", loops);
>               return i;
>            }
> -      }
> +      } else loops++;
>      }
> +printf("loops: %d (fail)\n", loops);
>      return -1;
>   }
>
> I get loops going up to 8386560. Which is roughly what I expected.
> This doesn't concern you at all?
>

I do agree that the count of iterated locations is high but because I 
get almost no difference whatsoever in time when doing shader_runner 
runs with this shader, I still consider the impact small.

I did few runs with ministat and the time difference is so small that 
ministat keeps saying "No difference proven at 95.0% confidence". This 
run takes ~17 secs on my IVB and around 10 secs on my HSW box. IIRC 
there's shaders in bugzilla that take 1 minute to compile and link.

In any case, I've reverted that patch as it had bugs and so that there 
is no regression and new implementation can be done in peace.

// Tapani