On Sat, Oct 31, 2015 at 11:31 AM, Connor Abbott <cwabbo...@gmail.com> wrote:
> On Sat, Oct 31, 2015 at 10:55 AM, Rob Clark <robdcl...@gmail.com> wrote:
>> On Fri, Oct 30, 2015 at 6:42 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>>> On Fri, Oct 30, 2015 at 2:12 PM, Ian Romanick <i...@freedesktop.org> wrote:
>>>> On 10/28/2015 10:01 PM, Jason Ekstrand wrote:
>>>>>
>>>>> On Oct 28, 2015 9:12 PM, "Kenneth Graunke" <kenn...@whitecape.org
>>>>> <mailto:kenn...@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
>>>>> <ja...@jlekstrand.net <mailto:ja...@jlekstrand.net>> wrote:
>>>>>> > > On Wed, Oct 28, 2015 at 2:32 PM, Jason Ekstrand
>>>>> <ja...@jlekstrand.net <mailto:ja...@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 <k...@bitplanet.net
>>>>> <mailto:k...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
