[Mesa-dev] Representing explicit memory layouts in NIR

apinheiro apinheiro at igalia.com
Sat Dec 1 10:05:51 UTC 2018


On 30/11/18 23:52, Jason Ekstrand wrote:
> On Fri, Nov 30, 2018 at 4:39 PM Timothy Arceri <tarceri at itsqueeze.com
> <mailto:tarceri at itsqueeze.com>> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181201/02a5fd65/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pEpkey.asc
Type: application/pgp-keys
Size: 1546 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181201/02a5fd65/attachment-0001.key>


More information about the mesa-dev mailing list