On Fri, Nov 30, 2018 at 4:39 PM Timothy Arceri <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. > > > > ## 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. --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 >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev