[Mesa-dev] [RFC 2/2] mesa: Add {Num}UniformBlocks and {Num}ShaderStorageBlocks to gl_shader_program

[Iago Toral]

On Thu, 2015-10-01 at 14:01 -0400, Ilia Mirkin wrote:
> On Thu, Oct 1, 2015 at 7:09 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
> > These arrays provide backends with separate index spaces for UBOS and SSBOs.
> > ---
> >  src/glsl/linker.cpp                 | 35 +++++++++++++++++++++++++++++++++++
> >  src/glsl/standalone_scaffolding.cpp |  9 +++++++++
> >  src/mesa/main/mtypes.h              |  6 ++++++
> >  3 files changed, 50 insertions(+)
> >
> > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > index e6eba94..3da773d 100644
> > --- a/src/glsl/linker.cpp
> > +++ b/src/glsl/linker.cpp
> > @@ -4107,6 +4107,41 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> >        }
> >     }
> >
> > +   /* Split prog->BufferInterfaceBlocks into prog->UniformBlocks and
> > +    * prog->ShaderStorageBlocks, so that drivers that need separate index
> > +    * spaces for each set can have that.
> > +    */
> > +   unsigned num_ubo_blocks;
> > +   unsigned num_ssbo_blocks;
> > +   num_ubo_blocks = 0;
> > +   num_ssbo_blocks = 0;
> > +   for (unsigned i = 0; i < prog->NumBufferInterfaceBlocks; i++) {
> > +      if (prog->BufferInterfaceBlocks[i].IsShaderStorage)
> > +         num_ssbo_blocks++;
> > +      else
> > +         num_ubo_blocks++;
> > +   }
> > +
> > +   prog->UniformBlocks =
> > +      ralloc_array(mem_ctx, gl_uniform_block *, num_ubo_blocks);
> > +   prog->NumUniformBlocks = 0;
> > +
> > +   prog->ShaderStorageBlocks =
> > +      ralloc_array(mem_ctx, gl_uniform_block *, num_ssbo_blocks);
> > +   prog->NumShaderStorageBlocks = 0;
> > +
> > +   for (unsigned i = 0; i < prog->NumBufferInterfaceBlocks; i++) {
> > +      if (prog->BufferInterfaceBlocks[i].IsShaderStorage)
> > +         prog->ShaderStorageBlocks[prog->NumShaderStorageBlocks++] =
> > +            &prog->BufferInterfaceBlocks[i];
> > +      else
> > +         prog->UniformBlocks[prog->NumUniformBlocks++] =
> > +            &prog->BufferInterfaceBlocks[i];
> > +   }
> 
> Shouldn't this go through and also adjust the indices of the linked
> programs? Or... something along those lines? With this, I still need a
> remapping table to go from the index passed to a load_ssbo/load_ubo
> instruction to a ssbo/ubo index.

Maybe I am missing something, but in the case of i965 for example, we
use a lowering pass (lower_ubo_reference) to compute the block indices
(see src/glsl/lower_ubo_reference.cpp). If you look at
lower_ubo_reference_visitor::setup_for_load_or_store, there is the code
we use to compute the block index. This happens in the backend already,
so I was thinking that you would do something similar, but instead of
using BufferInterfaceBlocks you would use these two arrays instead.
Until this point there are no references to any blocks in the shaders.

> Perhaps a few well-placed helper functions can alleviate this? Also
> this should erase the need for some of the O(n) iterations that have
> sprung up as a result of this combined list.
> 
> IMHO ideally the BufferInterfaceBlocks list would get freed at the end
> of this function. But I understand that this will require work, and
> the onus is probably on me (or anyone wanting to add ssbo support to
> other drivers) to do it, or work around it.

Yeah, it could go away I guess, but as you say that would require some
extra work (and also rewrite the i965 driver first to use separate index
spaces). In any case, notice that UniformBlocks and ShaderStorageBlocks
only contain pointers into BufferInterfaceBlocks, so we would only be
saving a few bytes.

Iago

>   -ilia
> 
> > +
> > +   assert(prog->NumUniformBlocks + prog->NumShaderStorageBlocks ==
> > +          prog->NumBufferInterfaceBlocks);
> > +
> >     /* FINISHME: Assign fragment shader output locations. */
> >
> >  done:
> > diff --git a/src/glsl/standalone_scaffolding.cpp b/src/glsl/standalone_scaffolding.cpp
> > index 0c53589..658245f 100644
> > --- a/src/glsl/standalone_scaffolding.cpp
> > +++ b/src/glsl/standalone_scaffolding.cpp
> > @@ -102,6 +102,15 @@ _mesa_clear_shader_program_data(struct gl_shader_program *shProg)
> >     ralloc_free(shProg->BufferInterfaceBlocks);
> >     shProg->BufferInterfaceBlocks = NULL;
> >     shProg->NumBufferInterfaceBlocks = 0;
> > +
> > +   ralloc_free(shProg->UniformBlocks);
> > +   shProg->UniformBlocks = NULL;
> > +   shProg->NumUniformBlocks = 0;
> > +
> > +   ralloc_free(shProg->ShaderStorageBlocks);
> > +   shProg->ShaderStorageBlocks = NULL;
> > +   shProg->NumShaderStorageBlocks = 0;
> > +
> >     for (i = 0; i < MESA_SHADER_STAGES; i++) {
> >        ralloc_free(shProg->UniformBlockStageIndex[i]);
> >        shProg->UniformBlockStageIndex[i] = NULL;
> > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> > index 347da14..2362f54 100644
> > --- a/src/mesa/main/mtypes.h
> > +++ b/src/mesa/main/mtypes.h
> > @@ -2692,6 +2692,12 @@ struct gl_shader_program
> >     unsigned NumBufferInterfaceBlocks;
> >     struct gl_uniform_block *BufferInterfaceBlocks;
> >
> > +   unsigned NumUniformBlocks;
> > +   struct gl_uniform_block **UniformBlocks;
> > +
> > +   unsigned NumShaderStorageBlocks;
> > +   struct gl_uniform_block **ShaderStorageBlocks;
> > +
> >     /**
> >      * Indices into the _LinkedShaders's UniformBlocks[] array for each stage
> >      * they're used in, or -1.
> > --
> > 1.9.1
> >
>