On 30/11/18 23:52, Jason Ekstrand wrote: > On Fri, Nov 30, 2018 at 4:39 PM Timothy Arceri <[email protected] > <mailto:[email protected]>> wrote: > > On 1/12/18 9:11 am, Jason Ekstrand wrote: > > All, > > > > This week, I've been working on trying to move UBO and SSBO > access in > > NIR over to deref instructions. I'm hoping that this will allow > us to > > start doing alias analysis and copy-propagation on it. The > passes we > > have in NIR *should* be able to work with SSBOs as long as > > nir_compare_derefs does the right thing. > > > > # A story about derefs > > > > In that effort, I've run into a bit of a snag with how to > represent the > > layout information. What we get in from SPIR-V for Vulkan is a > byte > > offset for every struct member and a byte stride for every array > (and > > pointer in the OpPtrAccessChain case). For matrices, there is an > > additional RowMajor boolean we need to track somewhere. With > OpenCL > > memory access, you don't get these decorations but it's trivial to > > translate the OpenCL layout (It's the same as C) to > offset/stride when > > creating the type. I've come up with three different ways to > represent > > the information and they all have their own downsides: > > > > ## 1. Put the information on the glsl_type similar to how it's > done in > > SPIR-V > > > > This has the advantage of being fairly non-invasive to > glsl_type. A lot > > of the fields we need are already there and the only real change > is to > > allow array types to have strides. The downside is that the > information > > is often not where you want. Arrays and structs are ok but, for > > matrices, you have to go fishing all the way back to the struct > type to > > get the RowMajor and MatrixStride decorations. (Thanks, > SPIR-V...) > > While this seems like a local annoyance, it actually destroys > basically > > all the advantages of having the information on the type and makes > > lower_io a real pain. > > > > ## 2. Put the information on the type but do it properly > > > > In this version, we would put the matrix stride and RowMajor > decoration > > directly on the matrix type. One obvious advantage here is that it > > means no fishing for matrix type information. Another is that, by > > having the types specialized like this, the only way to change > layouts > > mid-deref-chain would be to have a cast. Option 1 doesn't > provide this > > because matrix types are the same regardless of whether or not > they're > > declared RowMajor in the struct. The downside to this option is > that it > > requires glsl_type surgery to make it work. More on that in a bit. >
FWIW, while working on ARB_gl_spirv I also found annoying the need to
fish for the struct member information in order to get RowMajor and
MatrixStride information, as that lead to the need to track the
parent_type while you were processing the current type. As a example,
this is a extract of the code from _get_type_size (new method from
ARB_gl_spirv ubo/ssbo support, used to compute the ubo/ssbo size):
/* Matrices must have a matrix stride and either RowMajor or ColMajor */
if (glsl_type_is_matrix(type)) {
unsigned matrix_stride =
glsl_get_struct_field_explicit_matrix_stride(parent_type,
index_in_parent);
bool row_major =
glsl_get_struct_field_matrix_layout(parent_type,
index_in_parent) ==
GLSL_MATRIX_LAYOUT_ROW_MAJOR;
unsigned length = row_major ? glsl_get_vector_elements(type)
: glsl_get_length(type);
/* We don't really need to compute the type_size of the matrix element
* type. That should be already included as part of matrix_stride
*/
return matrix_stride * length;
}
So for a given type, sometimes we are using the info from the current
type, and sometimes we need to go up to the parent (so in addition to
the parent_type we also need to track the index of the current type on
its parent).
At some point I was tempted to move matrix stride and row major from the
struct field to glsl type. But as Jason mentioned, that would need a lot
of changes, and I felt that doing that only for ARB_gl_spirv convenience
was an overkill. But if we have now more reasons to do the move, I'm on
that ship.
> >
> > ## 3. Put the information directly on the deref
> >
> > Instead of putting the stride/offset information on the type, we
> just
> > put it on the deref as we build the deref chain. This is easy
> enough to
> > do in spirv_to_nir and someone could also do it easily enough as a
> > lowering pass based on a type_size function. This has the
> advantage of
> > simplicity because you don't have to modify glsl_type at all and
> > lowering is stupid-easy because all the information you need is
> right
> > there on the deref. The downside, however, is that you alias
> analysis
> > is potentially harder because you don't have the nice guarantee
> that you
> > don't see a layout change without a type cast. The other
> downside is
> > that we can't ever use copy_deref with anything bigger than a
> vector
> > because you don't know the sizes of any types and, unless
> spirv_to_nir
> > puts the offset/stride information on the deref, there's now way to
> > reconstruct it.
> >
> > I've prototyped both 1 and 3 so far and I definitely like 3
> better than
> > 1 but it's not great. I haven't prototyped 2 yet due to the issue
> > mentioned with glsl_type.
> >
> > Between 2 and 3, I really don't know how much we actually loose
> in terms
> > of our ability to do alias analysis. I've written the alias
> analysis
> > for 3 and it isn't too bad. I'm also not sure how much we would
> > actually loose from not being able to express whole-array or
> > whole-struct copies. However, without a good reason otherwise,
> option 2
> > really seems like it's the best of all worlds....
> >
> > # glsl_type surgery
> >
> > You want a good reason, eh? You should have known this was
> coming...
> >
> > The problem with option 2 above is that it requires significant
> > glsl_type surgery to do it. Putting decorations on matrices
> violates
> > one of the core principals of glsl_type, namely that all
> fundamental
> > types: scalars, vectors, matrices, images, and samplers are
> singletons.
> > Other types such as structs and arrays we build on-the-fly and
> cache
> > as-needed. In order to do what we need for option 2 above, you
> have to
> > at least drop this for matrices and possibly vectors (the
> columns of a
> > row-major mat4 are vectors with a stride of 16). Again, I see
> two options:
> >
> > ## A. Major rework of the guts of glsl_type
> >
> > Basically, get rid of the static singletons and just use the build
> > on-the-fly and cache model for everything. This would mean that
> mat4 ==
> > mat4 is no longer guaranteed unless you know a priori that none
> of your
> > types are decorated with layout information. It would also be,
> not only
> > a pile of work, but a single mega-patch. I don't know of any
> way to
> > make that change without just ripping it all up and putting it back
> > together.
>
> Do we really need to throw away the singleton model? Could we not add
> another type on top of matrices to hold the layout information
> much like
> how we handle arrays and structs and just strip it off (like we
> often do
> with arrays) when needed for comparisons?
>
> It's possible this could be messy, just trying to throw so ideas
> out there.
>
>
> As I've been thinking about that. My thought for nir_type (if that's
> a thing) has been to have a "bare type" pointer that always points
> back to the undecorated version to use in comparisons.
Wouldn't that be a really limited form of comparison compared with the
current glsl_type comparison? As you mention, right now several
validations checks are based on directly compare the types (type1 ==
type2), and right now that includes some decorations (just skimming
glsl_types::record_compare, we found explicit_xfb_buffer,
explicit_matrix_stride, etc). How would that work with the nir bare
type? It would be needed to do a two-step check? Or adding a comparison
method that compares recursively two full types?
>
> --Jason
>
>
> >
> > ## B. Make a new nir_type and make NIR use it
> >
> > This seems a bit crazy at this point. src/compiler/nir itself
> has over
> > 200 references to glsl_type and that doesn't include back-ends.
> It'd be
> > a major overhaul and it's not clear that it's worth it.
> However, it
> > would mean that we'd have a chance to rewrite types and maybe do it
> > better. Basing it on nir_alu_type instead of glsl_base_type
> would be
> > really nice because nir_alu_type already has an orthogonal split
> between
> > bit size and format (float, uint, etc.). I would also likely
> structure
> > it like vtn_type which has a different base_type concept which I
> find
> > works better than glsl_base_type.
> >
> > Of course, A would be less invasive than B but B would give us the
> > chance to improve some things without rewriting quite as many
> levels of
> > the compiler. There are a number of things I think we could do
> better
> > but changing those in the GLSL compiler would be a *lot* of work
> > especially since it doesn't use the C helpers that NIR does. On
> the
> > other hand, the churn in NIR from introducing a new type data
> structure
> > would be pretty big. I did a quick git grep and it looks like
> most of
> > the back-ends make pretty light use of glsl_type when it
> consuming NIR
> > so maybe it wouldn't be that bad?
> >
> > Thoughts? Questions? Objections?
> >
> > --Jason
>
pEpkey.asc
Description: application/pgp-keys
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
