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

[Connor Abbott]

On Sat, Oct 31, 2015 at 1:38 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Sat, Oct 31, 2015 at 11:31 AM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>> On Sat, Oct 31, 2015 at 10:55 AM, Rob Clark <robdclark at gmail.com> wrote:
>>> On Fri, Oct 30, 2015 at 6:42 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>> On Fri, Oct 30, 2015 at 2:12 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>>>> On 10/28/2015 10:01 PM, Jason Ekstrand wrote:
>>>>>>
>>>>>> On Oct 28, 2015 9:12 PM, "Kenneth Graunke" <kenneth at whitecape.org
>>>>>> <mailto:kenneth at whitecape.org>> wrote:
>>>>>>>
>>>>>>> On Wednesday, October 28, 2015 02:58:07 PM Kristian Høgsberg wrote:
>>>>>>> > On Wed, Oct 28, 2015 at 2:34 PM, Jason Ekstrand
>>>>>> <jason at jlekstrand.net <mailto:jason at jlekstrand.net>> wrote:
>>>>>>> > > On Wed, Oct 28, 2015 at 2:32 PM, Jason Ekstrand
>>>>>> <jason at jlekstrand.net <mailto:jason at jlekstrand.net>> 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.
>>>>>>> > >
>>>>>>> > > Once again, git-send-email failed to send the last patch for whatever
>>>>>>> > > reason.  The entire series can be found here:
>>>>>>> > >
>>>>>>> > > http://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/nir-pass
>>>>>>> >
>>>>>>> > Nice. Series,
>>>>>>> >
>>>>>>> > Reviewed-by: Kristian Høgsberg <krh at bitplanet.net
>>>>>> <mailto:krh at bitplanet.net>>
>>>>>>>
>>>>>>> I plan to review this as well, so please hold off on pushing it for a
>>>>>>> little while.  Thanks!
>>>>>>
>>>>>> By all means, go ahead.  It'd also be nice, while you're at it to weigh
>>>>>> in on how to handle passing arguments to passes.  There are a number of
>>>>>> ideas thrown back-and-forth between Connor and myself on how to do it
>>>>>
>>>>> Is it even necessary to sort that out now?  Are there passes that
>>>>> haven't been ported to this infrastructure that need any of the extra
>>>>> features that you and Connor were discussing?  I will give keithp credit
>>>>> for teaching me not to engineer in features that you don't need yet.
>>>>> When you do need them, the thing you spent a bunch of time putting in
>>>>> probably won't be what you need.
>>>>
>>>> Strictly speaking, no.  There are passes that haven't been converted
>>>> that will need it.  The issue is how do you pass arguments into
>>>> passes.  Most optimization passes don't care but there are a few that
>>>> will have non-trivial arguments.
>>>
>>> random drive-by comment:  in addition to making it easier to have pass
>>> specific args (which *could* I suppose be moved into
>>> nir_shader_compiler_options at the expense of making that not a const
>>> thing anymore, since some of the options depend on shader variant
>>> key), having old-fashioned code to call opt passes (instead of a table
>>> of passes) makes it easier to insert nir_print_shader() calls in
>>> strategic spots while debugging ;-)
>>
>> But with this, you can implement something like INTEL_DEBUG=opt where
>> you dump a file in between each pass and then you can diff them, which
>> is even better :).
>
> well, sometimes that is a bit more data than I want to collect ;-)

Well, you could always hack up the dumping routine to only dump on a
certain pass or shader name... but since the pass name and shader name
will be in the filename, you can also just use find/grep :) In my
personal experience fixing bugs in the i965 backend, being able to
dump out the result of all the optimization passes that made progress
was very useful. To get unique filenames, i965 uses a macro and a
counter variable, but it's cleaner if you have one place running all
the optimization passes since that place can update its counter
without having a macro that mucks with the variable behind the scenes.
Also, i965's implementation relies heavily on GCC statement
expressions, which we can't use in core NIR.

>
> Anyways, my natural reaction against 'meta-programming' type stuff
> (like, table of passes, vs calls to those passes) might be influenced
> by seeing some more horrible applications of the technique.  But I
> wonder if the same thing could be done w/ helper fxn instead.  At
> least in the current state I think it could, I mean:
>
>   // in nir_lower_foo.c:
>   ...
>   static nir_pass lower_foo = { ... };
>
>   nir_lower_foo(shader)
>   {
>      nir_shader_run_pass(shader, &lower_foo);
>   }
>
> That way, use the nir-pass helper stuff if it helps, or not.  Feels
> less like a mid-layer that way.  I think you could even implement your
> idea about optional cleanup-passes by adding 'struct nir_pass
> **cleanups' to nir_pass..

That's exactly what Jason's series already does :) There's nothing
preventing you from running individual optimization passes, or even
bypassing the whole thing entirely. But...

>
> From the consumer point of view, it looks like:
>
>     ...
>     nir_lower_foo(shader);
>     ...
>
> instead of:
>
>    ...
>    passes[num_passes++] = &lower_foo;
>    ...
>
> with the advantage that you can easily use
> loops/if-else/debug-prints/collect-stats/etc..

...most of these things can be done better with something like
nir_shader_optimize...

>
> Then again, I guess I get more or less the same thing if I just stick
> w/ calling nir_shader_run_pass() and ignore nir_shader_optimize()..

...so it would be very silly to do this. Having a list of cleanups
doesn't do you any good unless you can dynamically choose what to run
(rather than always just running it in a loop), and debug printing and
collecting statistics can be done better when you have one place that
knows about all the optimizations that are going to be run.

>
> BR,
> -R