<div dir="ltr"><div dir="ltr"><div dir="ltr"><div>For what it's worth, I've prototyped both 1 and 3 and I don't like either:</div><div><br></div><div><a href="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.2</a></div><div><a href="https://gitlab.freedesktop.org/jekstrand/mesa/commits/wip/nir-ubo-deref-v0.3">https://gitlab.freedesktop.org/jekstrand/mesa/commits/wip/nir-ubo-deref-v0.3</a></div><div><br></div><div>I'm going to try 2B next week.</div><div><br></div><div>--Jason</div><div><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Nov 30, 2018 at 4:11 PM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>All,</div><div><br></div><div>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.<br></div><div><br></div><div># A story about derefs<br></div><div><br></div><div>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:</div><div><br></div><div>## 1. Put the information on the glsl_type similar to how it's done in SPIR-V</div><div><br></div><div>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.</div><div><br></div><div>## 2. Put the information on the type but do it properly</div><div><br></div><div>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.</div><div><br></div><div>## 3. Put the information directly on the deref</div><div><br></div><div>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.</div><div><br></div><div>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.<br></div><div><br></div><div>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....</div><div><br></div><div># glsl_type surgery</div><div><br></div><div>You want a good reason, eh?  You should have known this was coming...</div><div><br></div><div>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:</div><div><br></div><div>## A. Major rework of the guts of glsl_type</div><div><br></div><div>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.</div><div><br></div><div>## B. Make a new nir_type and make NIR use it</div><div><br></div><div>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.</div><div><br></div><div>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?</div><div><br></div><div>Thoughts?  Questions?  Objections?<br></div><div><br></div><div>--Jason<br></div></div>
</blockquote></div>