On Sat, Oct 31, 2015 at 2:26 PM, Connor Abbott <cwabbo...@gmail.com> wrote: > On Sat, Oct 31, 2015 at 1:38 PM, Rob Clark <robdcl...@gmail.com> wrote: >> 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 ;-) > > 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...
umm.. well, if "better" means "less flexible".. since I assume you weren't planning to make nir_shader_optimize() turing complete.. >> >> 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. well unless you are thinking of deferring cleanups until end of optimize loop?? Otherwise there is nothing stopping nir_shader_run_pass() from conditionally running the cleanup passes. BR, -R _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
