For what it's worth, I've prototyped both 1 and 3 and I don't like either: https://gitlab.freedesktop.org/jekstrand/mesa/commits/wip/nir-ubo-deref-v0.2 https://gitlab.freedesktop.org/jekstrand/mesa/commits/wip/nir-ubo-deref-v0.3
I'm going to try 2B next week. --Jason On Fri, Nov 30, 2018 at 4:11 PM Jason Ekstrand <ja...@jlekstrand.net> 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. > > ## 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