On Tue, Nov 3, 2015 at 12:16 AM, Kenneth Graunke <kenn...@whitecape.org> wrote: > 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...
Yeah... arguments... I'll freely admit that that's the Achilles' heel of my whole plan. Unfortunately, the C language doesn't really provide good facilities for this. We could make it work less painfully with some fun GCC builtins, but I don't know how to do it on msvc or the like and even the builtins would be rather hackish. I guess it's something where if the advantages of having automatic pass management are high enough to offset the pain, we can switch then. > - 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. Yeah, I wasn't a fan of that either. > 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? My initial reaction was "but most passes don't take arguments". While I'd be ok with moving the bool from lower_vars_to_ssa into nir_compiler_options, that's about the only pass argument I'd be ok with moving there. Looking over things a bit more, it looks like about half of the lowering passes take arguments. (Substantially more than I was hoping) :-( > 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!) This is my #1 priority > 2. It removes some boilerplate for looping over impls. I like this, but it's not needed. You forgot my #2 priority: Automatic validation. > 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. I've reviewed most of those. Thanks for doing that. So, I still kind of like my struct... but I understand that there are problems with it that are, as of yet, unsolved. The patches I just sent are a rework of the nir_pass struct to make it basically just a helper for creating passes. In particular, I completely dropped nir_shader_optimize. I don't know how worth-while it is without that and without the function argument problems solved, but there it is none the less. --Jason _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
