<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 30/11/18 23:52, Jason Ekstrand wrote: </div> <blockquote type="cite" cite="mid:CAOFGe97hs+R_7C0yfG8WhUeP2TLAoKOmZ7OaKWWEXzmdCxhKOw@mail.gmail.com"> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> <div dir="ltr"> <div class="gmail_quote"> <div dir="ltr">On Fri, Nov 30, 2018 at 4:39 PM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com" moz-do-not-send="true">tarceri@itsqueeze.com</a>> wrote: </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. </blockquote> </div> </div> </blockquote> 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. <blockquote type="cite" cite="mid:CAOFGe97hs+R_7C0yfG8WhUeP2TLAoKOmZ7OaKWWEXzmdCxhKOw@mail.gmail.com"> <div dir="ltr"> <div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > ## 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. </blockquote> <div> </div> <div>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.</div> </div> </div> </blockquote> 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? <blockquote type="cite" cite="mid:CAOFGe97hs+R_7C0yfG8WhUeP2TLAoKOmZ7OaKWWEXzmdCxhKOw@mail.gmail.com"> <div dir="ltr"> <div class="gmail_quote"> <div> </div> <div>--Jason </div> <div> </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > ## 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 </blockquote> </div> </div> </blockquote> </body> </html>