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

[Rob Clark]

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

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

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

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

BR,
-R