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

[Kenneth Graunke]

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...

- 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.

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?

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!)

2. It removes some boilerplate for looping over impls.

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.

Sorry :(

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151103/f5f16f35/attachment.sig>