[Mesa-dev] [PATCH 7/7] nir: add helper macros for running NIR passes

[Jason Ekstrand]

On Wed, Oct 28, 2015 at 2:49 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Wed, Oct 28, 2015 at 5:24 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>> On Wed, Oct 28, 2015 at 1:37 PM, Rob Clark <robdclark at gmail.com> wrote:
>>> On Wed, Oct 28, 2015 at 12:09 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>> On Sat, Oct 24, 2015 at 10:08 AM, Rob Clark <robdclark at gmail.com> wrote:
>>>>> From: Rob Clark <robclark at freedesktop.org>
>>>>>
>>>>> Convenient place to put in some extra sanity checking, without making
>>>>> things messy for the drivers running the passes.
>>>>
>>>> In the short-term this seems to work (at least for testing nir_clone).
>>>> In the long-term, I'm not sure that a macro is really what we want.
>>>> I've mentioned a time or two before that what I *think* I'd like to do
>>>> (don't know exactly how it will work out yet) is to have a little
>>>> datastructure
>>>>
>>>> typedef struct nir_pass {
>>>>    bool (*shader_pass_func)(nir_shader *shader, void *data);
>>>>    bool (*impl_pass_func)(nir_function_impl *impl, void *data);
>>>>    nir_metadata metadata_preserved;
>>>>    void *data;
>>>> } nir_pass;
>>>>
>>>> and have each of the passes expose one of these as a const global
>>>> variable instead of exposing the actual functions.  Then we would have
>>>> a runner function (or macro) that could run a pass.  The runner would
>>>> take care of validation, trashing metadata, and maybe even cloning.
>>>> If no shader_pass_func is provided but you call it on a shader, the
>>>> runner would iterate over all of the overloads for you and run the
>>>> impl_pass_func on each.  We could also have helpers that take an array
>>>> and run all of them or even take an array and run it in a loop until
>>>> no more progress is made.
>>>
>>> meh, once we collapse the run+validate into a single line macro call,
>>> having list of calls sounds like it doesn't really take up more lines
>>> of code compared to a table of nir passes.. plus old fashioned code
>>> has a lot more flexibility without having to reinvent loops and ifs
>>> and that sort of thing.  Keep in mind some passes are conditional on
>>> draw state (ie. what we are lowering) or shader stage, etc.
>>>
>>> BR,
>>> -R
>>
>> FWIW, another reason that we might want to add something like this is
>> to optimize the ordering of passes so that they have to less work.
>> There are a lot of passes that act as "cleanups" for other passes; for
>> example, copy prop introduces a bunch of code that DCE has to clean
>> up. In addition, there are a lot of passes that are sort-of
>> "prerequisites" for another pass, doing some transform that lets
>> another pass do its work -- for example, lots of passes can't see
>> through copies and therefore require copy prop in order to do
>> anything, and deleting a trivial phi node may be necessary before we
>> can delete a loop. Right now, we try to add passes in more-or-less the
>> "right" order in the loop, but that's pretty icky and it's not obvious
>> to someone else using the infrastructure that a certain order might
>> not be optimal in terms of time required to get a fixed point.
>> Instead, I'd like for passes to be able to mark other passes as
>> prerequisites or cleanups, and have a scheduler/pass manager a la
>> LLVM's PassManager that tries to satisfy those dependencies (try and
>> run a cleanup pass if the previous pass reported progress, run passes
>> with unmet prerequisites last and passes with met prerequisites first,
>> etc.). Obviously, this is going to require some kind of pass struct
>> and some level of abstraction, although backends can still choose
>> which passes to add and they can still run passes themselves if they
>> so choose.
>
> interesting idea, and could make the effort worthwhile..
>
> still, however we end up doing this, it should be done in a way that
> we can replace the nir_shader to get nir_shader_clone() coverage.  I
> definitely think we want to have some built-in testability of clone.

It could be tweaked so that the runner takes a nir_shader ** so that
we can do that sort of thing.  I'm not sure how concerned I am about
continuous nir_shader_clone coverage but I'm ok with supporting it if
you'd like.  We can always pull it out once nir_shader_clone is used
enough places that we think it's getting tested ok.

> BR,
> -R
>
>>>
>>>
>>>> The thing I haven't quite settled on is how to pass extra parameters.
>>>> For some passes, we could just put the extra stuff in compiler_options
>>>> but we don't want to litter it too bad.  The other option is to do
>>>> what I did above and use the classic void pointer.  Then drivers would
>>>> have to just make a copy and set the data pointer to whatever they
>>>> want.
>>>>
>>>> Maybe I should just go implement this...
>>>>