<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<div class="moz-cite-prefix">On 1/12/18 14:34, Jason Ekstrand wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAOFGe95i=92ao3mpK9ETvV6Yi8b0WZR460sU9U7K0fY2qdBeZA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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" moz-do-not-send="true">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" moz-do-not-send="true">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>
</div>
</blockquote>
<p>Ok.</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CAOFGe95i=92ao3mpK9ETvV6Yi8b0WZR460sU9U7K0fY2qdBeZA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<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>
</div>
</blockquote>
<p><br>
</p>
<p>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. <br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CAOFGe95i=92ao3mpK9ETvV6Yi8b0WZR460sU9U7K0fY2qdBeZA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>Does that make more sense?</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>Yes, thanks for the explanation.</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CAOFGe95i=92ao3mpK9ETvV6Yi8b0WZR460sU9U7K0fY2qdBeZA@mail.gmail.com">
<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">
<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>
</blockquote>
</body>
</html>