On 1/12/18 14:34, Jason Ekstrand wrote: > On Sat, Dec 1, 2018 at 4:06 AM apinheiro <apinhe...@igalia.com > <mailto:apinhe...@igalia.com>> wrote: > > On 30/11/18 23:52, Jason Ekstrand wrote: >> On Fri, Nov 30, 2018 at 4:39 PM Timothy Arceri >> <tarc...@itsqueeze.com <mailto:tarc...@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: >> > > Something just occurred to me that I should note somewhere, so I'm > putting it here. Having static singletons for things like matrices is > actually useful for compiler performance. Because the cache is > global, every time we look a type up in the cache, we have to lock > around doing so. However, with the static singletons like > glsl_type::vec4_type, we can look them up without taking a lock. > Inside of NIR this probably isn't so bad because we don't tend to > construct a lot of types out of thin air; we tend to just use what's > there. However, for glsl_to_nir or spirv_to_nir, the cost would be > more noticeable. If we reworked glsl_type without a plan to solve > this problem, it would likely murder performance because that IR is > constantly creating temporary variables and needs types for them. I > see some python codegen in my future.... > > >> > ## 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? > > > Let me expand a bit on what I meant by that. Each type would have a > "bare_type" pointer. For bare types, it would either be NULL or point > to the type itself (I'm not sure which is more useful). If you made > an array of foo_type, for instance, foo_arr_type->bare_type would be > an array of foo_type->bare_type and not just an undecorated array of > foo_type. In a sense, the recursion is already done for you. This > would be sufficient for answering the question of "are these, in > general, the same type?" I'm not sure what I would do with struct > types. I'm a bit inclined to just say "same members in the same > order" and ignore things like field names but I'm not really sure.
Ok. > > If, on the other hand, you wanted to do something that looks more like > GLSL interface matching, you're on your own. You would have to write > a (probably recursive) function that applies all the fiddly little > rules around types and qualifiers. Ok. Right now I guess that this would not be needed for ARB_gl_spirv. Having said so, perhaps I'm wrong as now and then I found a new old-OpenGL semantic that I need to bring back. > > Does that make more sense? Yes, thanks for the explanation. > > --Jason > > >> >> --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 mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
