[Mesa-dev] [PATCH 1/7] linker: Split set_uniform_binding into separate functions for blocks and samplers

[Ian Romanick]

On 04/10/2014 12:04 PM, Ian Romanick wrote:
> On 04/09/2014 09:10 AM, Kenneth Graunke wrote:
>> On 04/04/2014 02:01 PM, Ian Romanick wrote:
>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>
>>> The two code paths are quite different, and there are some problems in
>>> the handling of uniform blocks.  Future changes will cause these paths
>>> to diverge further.  Ultimately, selecting between the two functions
>>> will happen at the set_uniform_binding call site, and
>>> set_uniform_binding will be deleted.
>>>
>>> NOTE: This patch just moves code around.
>>>
>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76323
>>> Cc: "10.1" <mesa-stable at lists.freedesktop.org>
>>> Cc: github at socker.lepus.uberspace.de
>>> ---
>>>  src/glsl/link_uniform_initializers.cpp | 42 +++++++++++++++++++++++++++++++---
>>>  1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> Assuming you have a reasonable response to my comment on patch 5, this
>> series is:
>>
>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>>
>> though, I'm not sure how much that's worth - I had to re-read the GLSL
>> rules and re-discover how our compiler IR for this stuff works.  The
>> code seems right, but I could be totally missing something obvious.
>>
>> On that note...is it just me, or is the compiler IR for uniform blocks
>> rather ugly and messy?
> 
> Yes.  Part of the problem is that we implemented things one "easy" way
> for GL_ARB_uniform_buffer_objects / OpenGL 3.1, but the addition of 3.2
> and OpenGL ES 3.0 features made the easy implementation not work.
> Rather that completely gut everything, I refactored a bunch of stuff
> and, basically, added a parallel implementation for the new bits.

A possible method of rectifying all of this just occured to me.  The
original problem was uniform blocks without an instance name put their
field names in global scope.  Applications do things like:

    uniform U { vec4 v; };

    void main()
    {
        gl_Position = v;
    }

We handle this by putting a 'vec4 v' variable at global scope.  This
variable contains additional information so that the uniform block
information can be found.

Blocks with instance names operate differently.

    uniform U { vec4 v; } u;

    void main()
    {
        gl_Position = u.v;
    }

In this case we put a 'U u' variable at global scope, and the u.v
dereference acts like a structure dereference.

What if we implemented something like the "using" keyword, and treat the
first case as if it were:

    uniform U { vec4 v; } __U;

    using __U;

    void main()
    {
        gl_Position = v;
    }

If a varible look-up fails, try each of the UBOs specified by using.  In
the IR, replace the dereference of v with __U.v.  When a new block is
added with using, there would need to be a check that none of its fields
conflict with globals or other blocks named with using.  Hmm... we'd
also have to check later globals for conflicts with using blocks.

I think that would solve a lot of the madness in the compiler and
linker.  There would still be some nonsense, IIRC, in the API side.  I
seem to recall that the API names vary depending on instance-name versus
non-instance-name.  That might actually be based on whether or not it's
a block array.  Hmm...

There's a couple possible warts, but it /seems/ like it may be cleaner
overall.  It should also localize the differences between the kinds of
blocks to the front end.

Thoughts?

>> Anyway, thanks a ton for doing this, Ian.  Sorry for dropping the ball
>> when we first implemented 420pack.
>>
>>> diff --git a/src/glsl/link_uniform_initializers.cpp b/src/glsl/link_uniform_initializers.cpp
>>> index 9d6977d..9a10350 100644
>>> --- a/src/glsl/link_uniform_initializers.cpp
>>> +++ b/src/glsl/link_uniform_initializers.cpp
>>> @@ -84,7 +84,7 @@ copy_constant_to_storage(union gl_constant_value *storage,
>>>  }
>>>  
>>>  void
>>> -set_uniform_binding(void *mem_ctx, gl_shader_program *prog,
>>> +set_sampler_binding(void *mem_ctx, gl_shader_program *prog,
>>>                      const char *name, const glsl_type *type, int binding)
>>>  {
>>>     struct gl_uniform_storage *const storage =
>>> @@ -95,7 +95,7 @@ set_uniform_binding(void *mem_ctx, gl_shader_program *prog,
>>>        return;
>>>     }
>>>  
>>> -   if (storage->type->is_sampler()) {
>>> +   {
>>>        unsigned elements = MAX2(storage->array_elements, 1);
>>>  
>>>        /* From section 4.4.4 of the GLSL 4.20 specification:
>>> @@ -118,7 +118,24 @@ set_uniform_binding(void *mem_ctx, gl_shader_program *prog,
>>>              }
>>>           }
>>>        }
>>> -   } else if (storage->block_index != -1) {
>>> +   }
>>> +
>>> +   storage->initialized = true;
>>> +}
>>> +
>>> +void
>>> +set_block_binding(void *mem_ctx, gl_shader_program *prog,
>>> +                  const char *name, const glsl_type *type, int binding)
>>> +{
>>> +   struct gl_uniform_storage *const storage =
>>> +      get_storage(prog->UniformStorage, prog->NumUserUniformStorage, name);
>>> +
>>> +   if (storage == NULL) {
>>> +      assert(storage != NULL);
>>> +      return;
>>> +   }
>>> +
>>> +   if (storage->block_index != -1) {
>>>        /* This is a field of a UBO.  val is the binding index. */
>>>        for (int i = 0; i < MESA_SHADER_STAGES; i++) {
>>>           int stage_index = prog->UniformBlockStageIndex[i][storage->block_index];
>>> @@ -134,6 +151,25 @@ set_uniform_binding(void *mem_ctx, gl_shader_program *prog,
>>>  }
>>>  
>>>  void
>>> +set_uniform_binding(void *mem_ctx, gl_shader_program *prog,
>>> +                    const char *name, const glsl_type *type, int binding)
>>> +{
>>> +   struct gl_uniform_storage *const storage =
>>> +      get_storage(prog->UniformStorage, prog->NumUserUniformStorage, name);
>>> +
>>> +   if (storage == NULL) {
>>> +      assert(storage != NULL);
>>> +      return;
>>> +   }
>>> +
>>> +   if (storage->type->is_sampler()) {
>>> +      set_sampler_binding(mem_ctx, prog, name, type, binding);
>>> +   } else if (storage->block_index != -1) {
>>> +      set_block_binding(mem_ctx, prog, name, type, binding);
>>> +   }
>>> +}
>>> +
>>> +void
>>>  set_uniform_initializer(void *mem_ctx, gl_shader_program *prog,
>>>  			const char *name, const glsl_type *type,
>>>  			ir_constant *val)
>>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140410/e93c277f/attachment.sig>