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

[Rob Clark]

On Wed, Oct 28, 2015 at 6:01 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> 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.

I think that would be a good idea.. as NIR evolves, I think it would
be good to have an easy way to ensure that clone doesn't break in
subtle ways..

BR,
-R