<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sat, Dec 1, 2018 at 4:06 AM apinheiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div class="m_-5999291674790634495moz-cite-prefix">On 30/11/18 23:52, Jason Ekstrand
wrote:<br>
</div>
<blockquote type="cite">
<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" target="_blank">tarceri@itsqueeze.com</a>>
wrote:<br>
</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:<br>
> All,<br>
> <br>
> This week, I've been working on trying to move UBO and
SSBO access in <br>
> NIR over to deref instructions. I'm hoping that this
will allow us to <br>
> start doing alias analysis and copy-propagation on it.
The passes we <br>
> have in NIR *should* be able to work with SSBOs as long
as <br>
> nir_compare_derefs does the right thing.<br>
> <br>
> # A story about derefs<br>
> <br>
> In that effort, I've run into a bit of a snag with how
to represent the <br>
> layout information. What we get in from SPIR-V for
Vulkan is a byte <br>
> offset for every struct member and a byte stride for
every array (and <br>
> pointer in the OpPtrAccessChain case). For matrices,
there is an <br>
> additional RowMajor boolean we need to track
somewhere. With OpenCL <br>
> memory access, you don't get these decorations but it's
trivial to <br>
> translate the OpenCL layout (It's the same as C) to
offset/stride when <br>
> creating the type. I've come up with three different
ways to represent <br>
> the information and they all have their own downsides:<br>
> <br>
> ## 1. Put the information on the glsl_type similar to
how it's done in <br>
> SPIR-V<br>
> <br>
> This has the advantage of being fairly non-invasive to
glsl_type. A lot <br>
> of the fields we need are already there and the only
real change is to <br>
> allow array types to have strides. The downside is
that the information <br>
> is often not where you want. Arrays and structs are ok
but, for <br>
> matrices, you have to go fishing all the way back to
the struct type to <br>
> get the RowMajor and MatrixStride decorations.
(Thanks, SPIR-V...) <br>
> While this seems like a local annoyance, it actually
destroys basically <br>
> all the advantages of having the information on the
type and makes <br>
> lower_io a real pain.<br>
> <br>
> ## 2. Put the information on the type but do it
properly<br>
> <br>
> In this version, we would put the matrix stride and
RowMajor decoration <br>
> directly on the matrix type. One obvious advantage
here is that it <br>
> means no fishing for matrix type information. Another
is that, by <br>
> having the types specialized like this, the only way to
change layouts <br>
> mid-deref-chain would be to have a cast. Option 1
doesn't provide this <br>
> because matrix types are the same regardless of whether
or not they're <br>
> declared RowMajor in the struct. The downside to this
option is that it <br>
> requires glsl_type surgery to make it work. More on
that in a bit.<br>
</blockquote>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>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):</p>
<p> /* Matrices must have a matrix stride and either RowMajor or
ColMajor */<br>
if (glsl_type_is_matrix(type)) {<br>
unsigned matrix_stride =<br>
glsl_get_struct_field_explicit_matrix_stride(parent_type,
index_in_parent);<br>
<br>
bool row_major =<br>
glsl_get_struct_field_matrix_layout(parent_type,
index_in_parent) ==<br>
GLSL_MATRIX_LAYOUT_ROW_MAJOR;<br>
<br>
unsigned length = row_major ? glsl_get_vector_elements(type)<br>
: glsl_get_length(type);<br>
<br>
/* We don't really need to compute the type_size of the
matrix element<br>
* type. That should be already included as part of
matrix_stride<br>
*/<br>
return matrix_stride * length;<br>
}<br>
</p>
<p><br>
</p>
<p>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).</p>
<p>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.<br>
</p>
<p><br>
</p>
<blockquote type="cite">
<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">
> <br>
> ## 3. Put the information directly on the deref<br>
> <br>
> Instead of putting the stride/offset information on the
type, we just <br>
> put it on the deref as we build the deref chain. This
is easy enough to <br>
> do in spirv_to_nir and someone could also do it easily
enough as a <br>
> lowering pass based on a type_size function. This has
the advantage of <br>
> simplicity because you don't have to modify glsl_type
at all and <br>
> lowering is stupid-easy because all the information you
need is right <br>
> there on the deref. The downside, however, is that you
alias analysis <br>
> is potentially harder because you don't have the nice
guarantee that you <br>
> don't see a layout change without a type cast. The
other downside is <br>
> that we can't ever use copy_deref with anything bigger
than a vector <br>
> because you don't know the sizes of any types and,
unless spirv_to_nir <br>
> puts the offset/stride information on the deref,
there's now way to <br>
> reconstruct it.<br>
> <br>
> I've prototyped both 1 and 3 so far and I definitely
like 3 better than <br>
> 1 but it's not great. I haven't prototyped 2 yet due
to the issue <br>
> mentioned with glsl_type.<br>
> <br>
> Between 2 and 3, I really don't know how much we
actually loose in terms <br>
> of our ability to do alias analysis. I've written the
alias analysis <br>
> for 3 and it isn't too bad. I'm also not sure how much
we would <br>
> actually loose from not being able to express
whole-array or <br>
> whole-struct copies. However, without a good reason
otherwise, option 2 <br>
> really seems like it's the best of all worlds....<br>
> <br>
> # glsl_type surgery<br>
> <br>
> You want a good reason, eh? You should have known this
was coming...<br>
> <br>
> The problem with option 2 above is that it requires
significant <br>
> glsl_type surgery to do it. Putting decorations on
matrices violates <br>
> one of the core principals of glsl_type, namely that
all fundamental <br>
> types: scalars, vectors, matrices, images, and samplers
are singletons. <br>
> Other types such as structs and arrays we build
on-the-fly and cache <br>
> as-needed. In order to do what we need for option 2
above, you have to <br>
> at least drop this for matrices and possibly vectors
(the columns of a <br>
> row-major mat4 are vectors with a stride of 16).
Again, I see two options:<br></blockquote></div></div></blockquote></div></blockquote><div><br></div><div>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....<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><blockquote type="cite"><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">
> ## A. Major rework of the guts of glsl_type<br>
> <br>
> Basically, get rid of the static singletons and just
use the build <br>
> on-the-fly and cache model for everything. This would
mean that mat4 == <br>
> mat4 is no longer guaranteed unless you know a priori
that none of your <br>
> types are decorated with layout information. It would
also be, not only <br>
> a pile of work, but a single mega-patch. I don't know
of any way to <br>
> make that change without just ripping it all up and
putting it back <br>
> together.<br>
<br>
Do we really need to throw away the singleton model? Could
we not add <br>
another type on top of matrices to hold the layout
information much like <br>
how we handle arrays and structs and just strip it off (like
we often do <br>
with arrays) when needed for comparisons?<br>
<br>
It's possible this could be messy, just trying to throw so
ideas out there.<br>
</blockquote>
<div><br>
</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>
<p><br>
</p>
<p>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?</p></div></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Does that make more sense?</div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>--Jason<br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> <br>
> ## B. Make a new nir_type and make NIR use it<br>
> <br>
> This seems a bit crazy at this point. src/compiler/nir
itself has over <br>
> 200 references to glsl_type and that doesn't include
back-ends. It'd be <br>
> a major overhaul and it's not clear that it's worth
it. However, it <br>
> would mean that we'd have a chance to rewrite types and
maybe do it <br>
> better. Basing it on nir_alu_type instead of
glsl_base_type would be <br>
> really nice because nir_alu_type already has an
orthogonal split between <br>
> bit size and format (float, uint, etc.). I would also
likely structure <br>
> it like vtn_type which has a different base_type
concept which I find <br>
> works better than glsl_base_type.<br>
> <br>
> Of course, A would be less invasive than B but B would
give us the <br>
> chance to improve some things without rewriting quite
as many levels of <br>
> the compiler. There are a number of things I think we
could do better <br>
> but changing those in the GLSL compiler would be a
*lot* of work <br>
> especially since it doesn't use the C helpers that NIR
does. On the <br>
> other hand, the churn in NIR from introducing a new
type data structure <br>
> would be pretty big. I did a quick git grep and it
looks like most of <br>
> the back-ends make pretty light use of glsl_type when
it consuming NIR <br>
> so maybe it wouldn't be that bad?<br>
> <br>
> Thoughts? Questions? Objections?<br>
> <br>
> --Jason<br>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote></div></div>