[Mesa-dev] [PATCH 00/11] nir: Add a pass management framework

[Jason Ekstrand]

On Tue, Nov 3, 2015 at 12:16 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, October 28, 2015 02:32:00 PM Jason Ekstrand wrote:
>> This series adds a nir_pass datastructure and some helpers for managing
>> optimization and lowering passes.  I've been meaning to get around to this
>> for some time.  There are a couple of primary benifits to this:
>>
>> First, this gives us a central place to put things such as validating the
>> shader, printing it if it changes, etc.  Right now, the i965 backend calls
>> nir_validate_shader after each pass.  We would also like to add something
>> like we have in the i965 backend where it can be set to dump the IR to a
>> file after every pass that changess it.
>>
>> Mor importantly, though, it moves metadata out of the passes them selves
>> and into the runner.  In the process of putting this series together, I
>> found at least 3 or 4 optimization passes that don't properly invalidate
>> metadata.  By putting a metadata_preserved field in nir_pass and handling
>> metadata in the pass runner, we make it much less likely that a pass will
>> get this wrong.  LLVM has a similar optimization pass architecture for
>> precicely this reason.
>>
>> As a nice little side-benifit, we no longer have to iterate over all of the
>> overloads with non-NULL impl pointers in each pass.
>
> I read through the series today.  Most of it seemed pretty reasonable,
> but when I got to patch 11 and saw the final result...I just wasn't a
> huge fan of how it turned out.
>
> There's something really nice about having passes simply be functions
> that manipulate the shader.  They can easily take arguments.  You can
> easily call them conditionally - which we do, based on scalar-vs-vec4,
> generation number, or shader stage.  You can assemble passes into groups
> by using helper functions (i.e. brw_nir_lower_inputs).  You can easily
> add breakpoints at arbitrary locations.
>
> While you can technically still do these things with the flat array of
> structs, I think it would be awkward in practice:
>
> - Packing a bunch of function arguments into a struct, passing them as
>   a void *, and unpacking them is a bunch of boilerplate.  Especially
>   for passes that have a single parameter, i.e. "true"/"false" or "32".
>
> - For passes that don't have any arguments, adding the extra unused
>   (void *closure) parameter also adds more boilerplate.  Plus, it just
>   seems unfortunate - they already had a nice clean API...

Yeah... arguments... I'll freely admit that that's the Achilles' heel
of my whole plan.  Unfortunately, the C language doesn't really
provide good facilities for this.  We could make it work less
painfully with some fun GCC builtins, but I don't know how to do it on
msvc or the like and even the builtins would be rather hackish.  I
guess it's something where if the advantages of having automatic pass
management are high enough to offset the pain, we can switch then.

> - Conditionally calling passes means assembling a list of passes on the
>   fly, dynamically, rather than using a static array.  Would probably
>   get messy.  Hope the array sizes are right.

Yeah, I wasn't a fan of that either.

> Also, in order for INTEL_DEBUG=optimizer style output to work, *all*
> lowering/optimization passes need to participate.  This lets you diff
> successive code dumps, seeing what each pass did.  If some passes don't
> get printed, then the output is unintelligable.  (I've had to fix that.)
>
> Notably, patch 11 doesn't convert any passes with arguments or
> conditional passes to the infrastructure - only the simple cases.
> Will this work okay for the more complex ones, or will we want
> something different?  Is this a step toward that?

My initial reaction was "but most passes don't take arguments".  While
I'd be ok with moving the bool from lower_vars_to_ssa into
nir_compiler_options, that's about the only pass argument I'd be ok
with moving there.  Looking over things a bit more, it looks like
about half of the lowering passes take arguments. (Substantially more
than I was hoping) :-(

> I also tend to agree with Rob's reluctance toward meta-programming,
> FWIW...I'd at least like to be wowed by some nice benefits.
>
> It sounds like Jason and Connor have some ideas about how a pass
> manager could be helpful in the future.  But for now, I only see
> two concrete benefits:
>
> 1. It ensures nir_metadata_preserve gets called appropriately.
>    (this is *really* important!)

This is my #1 priority

> 2. It removes some boilerplate for looping over impls.

I like this, but it's not needed.

You forgot my #2 priority: Automatic validation.

> I have an alternate approach to suggest for #1.  I don't think #2 is
> terribly important for now, since Jason may end up reworking functions
> a bit as part of his SPIR-V work...and it seems like we should defer
> tidying until we have more than one function.
>
> Also, we do need to fix missing nir_metadata_preserve calls on the
> stable branch, so we should fix those independently of implementing
> a new pass management scheme.  I have patches for that.

I've reviewed most of those. Thanks for doing that.

So, I still kind of like my struct... but I understand that there are
problems with it that are, as of yet, unsolved.

The patches I just sent are a rework of the nir_pass struct to make it
basically just a helper for creating passes.  In particular, I
completely dropped nir_shader_optimize.  I don't know how worth-while
it is without that and without the function argument problems solved,
but there it is none the less.
--Jason