[Mesa-dev] SSBO's in UniformBlocks list?

[Iago Toral]

On Wed, 2015-09-30 at 11:54 +0300, Francisco Jerez wrote:
> Iago Toral <itoral at igalia.com> writes:
> 
> > On Tue, 2015-09-29 at 18:41 +0300, Francisco Jerez wrote:
> >> Ilia Mirkin <imirkin at alum.mit.edu> writes:
> >> 
> >> > On Tue, Sep 29, 2015 at 4:33 AM, Iago Toral <itoral at igalia.com> wrote:
> >> >> Hi ilia,
> >> >>
> >> >> On Tue, 2015-09-29 at 03:53 -0400, Ilia Mirkin wrote:
> >> >>> Hi Samuel, and any other onlookers,
> >> >>>
> >> >>> I was wondering why the decision was made to stick SSBO's onto the
> >> >>> same list as constbufs. Seems like they're entirely separate entities,
> >> >>> no? Perhaps I'm missing something?
> >> >>
> >> >> The reason for this was that there is a lot of code in the compiler to
> >> >> handle uniform blocks and all the rules for them and we needed the same
> >> >> treatment for SSBOs, so that seemed like a reasonable way forward to
> >> >> reuse a lot of the code in the compiler front end. I think the only
> >> >> place where we needed to make explicit distinctions is when we check for
> >> >> resource limits, since these are different for UBOs and SSBOs of course.
> >> >> Although UBOs and SSBOs are separate entities they have a lot of
> >> >> similarities too, so that did not look like a terrible idea, considering
> >> >> the benefits.
> >> >
> >> > My concern is around indexing... now the per-stage indices are in the
> >> > combined UBO/SSBO space -- how do I tease out the individual ones?
> >> > Easy enough when you can loop over NumUniformBlocks and just count the
> >> > right type, but what about in the shader, where I get the buffer index
> >> > in a ir_rvalue?
> >> >
> >> Yeah, this seems rather dubious to me too.  Even if you had re-used the
> >> current gl_uniform_block type for SSBOs for the sake of code-sharing I
> >> think it would have made more sense to split them into a different index
> >> space, because SSBOs are a different index space at the API level and
> >> drivers will want them to be a different index space too.
> >
> > In the case of i965 at least I think having them combined makes things
> > easier because from the driver perspective the only difference between
> > SSBOs and UBOs is that the underlying buffer storage has different
> > flags. This was the simplest implementation and in theory, translating
> > from the shared space to a separate space should be very easy too, so
> > drivers that need a separate space should be able to get that as well...
> > That's in theory, the problem is that Ilia is saying that in his case
> > the compiler backend may not have the information required to make this
> > translation, and if this is true then we'll have to rethink this...
> >
> I think this is largely irrelevant for the i965 driver, code can be
> shared either way if shader UBOs and SSBOs are represented using the
> same data type: Assuming that function f() is shared among SSBOs and
> UBOs, it could also have been shared with separate arrays by changing it
> into f(gl_uniform_block[]) and passing the right U/SSBO array as
> argument.
> 
> Translating from a separate space to a shared index space is trivial and
> has O(1) complexity (shared_ssbo_index = separate_ssbo_index +
> max_ubo_index), although it's unlikely to be necessary on any HW
> arrchitecture I know of.  OTOH translating from the shared space to a
> separatee space (as the GL API and Gallium drivers require) involves
> iterating over all previous U/SSBOs of the shader and has O(n)
> complexity (e.g. as you do here [1]).

However, this will come at the expense of having to modify the compiler
front-end since that assumes that we only have one array at the moment
and all the code that deals with uniform blocks works with that
assumption. Not that we can't do this of course, but I was hoping that
we could avoid it since that part of the compiler is complex enough as
it is... Anyway, at this point I guess the best thing I can do is to
implement the separate index space and see that it looks like,
hopefully it is not that bad and if gallium drivers can't translate from
the shared index space we don't have an alternative anyway.

Iago

> [1] https://patchwork.freedesktop.org/patch/60654/
> 
> >> I believe that this leads to a bug the i965 implementation -- We expose
> >> 12 SSBOs per stage and 12 UBOs per stage, but we only have 12 binding
> >> table entries reserved for the block of the binding table currently
> >> shared among UBOs and SSBOs, so you might overflow the number of
> >> available surface entries if the combined number of UBOs and SSBOs is
> >> greater than 12 for some stage.
> >
> > Yeah, we forgot to update that. I'll send a patch to fix this. Thanks
> > Curro!
> >
> > Iago
> >
> >> > Thanks,
> >> >
> >> >   -ilia
> >> > _______________________________________________
> >> > mesa-dev mailing list
> >> > mesa-dev at lists.freedesktop.org
> >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev