[Mesa-dev] [RFC] NIR serialization

[Connor Abbott]

me too :) I'll push my stuff now

On Thu, Sep 14, 2017 at 8:58 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Thu, Sep 14, 2017 at 12:40 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>
>> On Tue, Sep 12, 2017 at 2:09 PM, Jason Ekstrand <jason at jlekstrand.net>
>> wrote:
>> > On Tue, Sep 12, 2017 at 10:12 AM, Ian Romanick <idr at freedesktop.org>
>> > wrote:
>> >>
>> >> On 09/11/2017 11:17 PM, Kenneth Graunke wrote:
>> >> > On Monday, September 11, 2017 9:23:05 PM PDT Ian Romanick wrote:
>> >> >> On 09/08/2017 01:59 AM, Kenneth Graunke wrote:
>> >> >>> On Thursday, September 7, 2017 4:26:04 PM PDT Jordan Justen wrote:
>> >> >>>> On 2017-09-06 14:12:41, Daniel Schürmann wrote:
>> >> >>>>> Hello together!
>> >> >>>>> Recently, we had a small discussion (off the list) about the NIR
>> >> >>>>> serialization, which was previously discussed in [RFC]
>> >> >>>>> ARB_gl_spirv
>> >> >>>>> and
>> >> >>>>> NIR backend for radeonsi.
>> >> >>>>>
>> >> >>>>> As this topic could be interesting to more people, I would like
>> >> >>>>> to
>> >> >>>>> share, what was talked about so far (You might want to read from
>> >> >>>>> bottom up).
>> >> >>>>>
>> >> >>>>> TL;DR:
>> >> >>>>> - NIR serialization is in demand for shader cache
>> >> >>>>> - could be done either directly (NIR binary form) or via SPIR-V
>> >> >>>>> - Ian et al. are working on GLSL IR -> SPIR-V transformation,
>> >> >>>>> which
>> >> >>>>> could be adapted for a NIR -> SPIR-V pass
>> >> >>>>> - in NIR representation, some type information is lost
>> >> >>>>> - thus, a serialization via SPIR-V could NOT be a glslang
>> >> >>>>> alternative
>> >> >>>>> (otoh, the GLSL IR->SPIR-V pass could), but only for spirv-opt
>> >> >>>>> (if
>> >> >>>>> the
>> >> >>>>> output is valid SPIR-V)
>> >> >>>>
>> >> >>>> Ian,
>> >> >>>>
>> >> >>>> Tim was suggesting that we might look at serializing nir for the
>> >> >>>> i965
>> >> >>>> shader cache. Based on this email, it sounds like serialized nir
>> >> >>>> would
>> >> >>>> not be enough for the shader cache as some GLSL type info would be
>> >> >>>> lost. It sounds like GLSL IR => SPIR-V would be good enough. Is
>> >> >>>> that
>> >> >>>> right?
>> >> >>>>
>> >> >>>> I don't think we have a strict requirement for the GLSL IR =>
>> >> >>>> SPIR-V
>> >> >>>> path for GL 4.6, right? So, this is more of a 'nice-to-have'?
>> >> >>>>
>> >> >>>> I'm not sure we'd want to make i965 shader cache depend on a
>> >> >>>> nice-to-have feature. (Unless we're pretty sure it'll be available
>> >> >>>> soon.)
>> >> >>>>
>> >> >>>> But, it would be nice to not have to fallback to compiling the
>> >> >>>> GLSL
>> >> >>>> for i965 shader cache, so it would be worth waiting a little bit
>> >> >>>> to
>> >> >>>> be
>> >> >>>> able to rely on a SPIR-V serialization of the GLSL IR.
>> >> >>>>
>> >> >>>> What do you suggest?
>> >> >>>>
>> >> >>>> -Jordan
>> >> >>>
>> >> >>> We shouldn't use SPIR-V for the shader cache.
>> >> >>>
>> >> >>> The compilation process for GLSL is: GLSL -> GLSL IR -> NIR -> i965
>> >> >>> IRs.
>> >> >>> Storing the content at one of those points, and later loading it
>> >> >>> and
>> >> >>> resuming the normal compilation process from that point...that's
>> >> >>> totally
>> >> >>> reasonable.
>> >> >>>
>> >> >>> Having a fallback for "some things in the cache but not all the
>> >> >>> variants
>> >> >>> we needed" suddenly take a different compilation pipeline, i.e.
>> >> >>> SPIR-V
>> >> >>> -> NIR -> ... seems risky.  It's a different compilation path that
>> >> >>> we
>> >> >>> don't normally use.  And one you'd only hit in limited
>> >> >>> circumstances.
>> >> >>> There's a lot of potential for really obscure bugs.
>> >> >>
>> >> >> Since we're going to expose exactly that path for GL_ARB_spirv /
>> >> >> OpenGL
>> >> >> 4.6, we'd better make sure it works always.  Right?
>> >> >
>> >> > In addition to the old pipeline:
>> >> >
>> >> > - GLSL from the app -> GLSL IR -> NIR -> i965 IR
>> >> >
>> >> > GL_ARB_spirv and OpenGL 4.6 add a second pipeline:
>> >> >
>> >> > - SPIR-V from the app -> NIR -> i965 IR
>> >> >
>> >> > Both of those absolutely have to work.  But these:
>> >> >
>> >> > - GLSL -> GLSL IR -> NIR -> SPIR-V -> NIR -> i965 IRs
>> >> > - GLSL -> GLSL IR -> SPIR-V -> NIR -> i965 IRs
>> >> >
>> >> > aren't required to work, or even be supported.  It makes a lot of
>> >> > sense
>> >> > to support them - both for testing purposes, and as an alternative to
>> >> > glslang, for a broader tooling ecosystem.
>> >> >
>> >> > The thing that concerns me is that if you use SPIR-V for the cache,
>> >> > you
>> >> > need these paths to not just work, but be _indistinguishable_ from
>> >> > one
>> >> > another:
>> >> >
>> >> > - GLSL -> GLSL IR -> NIR -> ...
>> >> > - GLSL -> GLSL IR -> NIR -> SPIR-V, then SPIR-V -> NIR -> ...
>> >> >
>> >> > Otherwise the original compile and partially-cached recompile might
>> >> > have
>> >> > different properties.  For example, if the the SPIR-V step messes
>> >> > with
>> >> > variables or instruction ordering a little, it could trip up the loop
>> >> > unroller so the original compiler gets unrolled, and the recompile
>> >> > from
>> >> > partial cache doesn't get unrolled.  I don't want to have to debug
>> >> > that.
>> >>
>> >> That is a very compelling argument.  If we want Mesa to be an
>> >> alternative to glslang, I think we would like to have that property,
>> >> but
>> >> it's not a hard requirement for that use case.
>> >
>> >
>> > I also find that argument rather compelling.  The SPIR-V -> NIR pass is
>> > *not* a simple pass.  It does piles of lowering and things on-the-fly as
>> > well as creating temporary variables for various things.  The best we
>> > could
>> > hope to guarnatee would be that NIR -> SPIR-V -> NIR -> vars_to_ssa ->
>> > CSE
>> > is idempotent.  Even that might be a bit of a stretch.
>> >
>> >>
>> >> > One could avoid this by making the original compile always go through
>> >> > SPIR-V, and just drop glsl_to_nir altogether, so both take the same
>> >> > paths.  But...it's kind of an unnecessary step in the common case...
>> >>
>> >> We may eventually partially do that, but that shouldn't block (any)
>> >> other work.  In the short term it would likely add compile overhead
>> >> that
>> >> many would find unacceptable... by virtue of being non-zero.
>> >>
>> >> > Just serializing/reading back the NIR and resuming the compile from
>> >> > the
>> >> > exact same IR would also solve that problem.
>> >> >
>> >> > Or, just being -really- careful with the translator, I guess...
>> >> >
>> >> >> One nice thing about SPIR-V is that all of the handling of uniform
>> >> >> layouts, initial uniform values, attribute locations, etc. is
>> >> >> already
>> >> >> serialized.  If I'm not mistaken, that was one of the big pain
>> >> >> points
>> >> >> for all of the existing on-disk storage methods.  All of that has
>> >> >> been
>> >> >> sorted out for SPIR-V, and we have to make it work anyway.
>> >> >
>> >> > That is pretty nice.  I don't recall it being that painful, but, not
>> >> > reinventing things is kind of nice too...
>> >>
>> >> Maybe the right answer is to share some things from SPIR-V (e.g., the
>> >> way it describes I/O) to reduce duplication, but serialize NIR
>> >> instructions and control flow in a "native" format.
>> >
>> >
>> > I strongly suspect that there is some overestimation of how much code
>> > nir_serialize would actually be here.  I just looked at nir_clone.c and
>> > it's
>> > 781 lines.  It could probably drop by 50-100 LOC if we didn't expose
>> > helpers
>> > for also cloning of single functions, variables, and constants.  I would
>> > expect nir_serialize to be able to handle serialization and
>> > deserialization
>> > in about 2x the code of nir_clone.c.  By comparison, SPIR-V -> NIR is
>> > 8159
>> > LOC and counting (that doesn't include generated anything) and I would
>> > expect GLSL -> SPIR-V to be similarly sized.
>>
>> I just finished typing up a nir_serialize implementation, although I
>> haven't debugged it yet. It wound up being 1150 lines, including
>> comments and whatnot:
>>
>> https://cgit.freedesktop.org/~cwabbott0/mesa/commit/?h=nir-serialize&id=2bacd646460328940c5021d1bdaced09a45ed947
>
>
> Taking the lock... I've got a test harness written for it and am working on
> fixing some of the bugs. :-)