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

[Jason Ekstrand]

On Wed, Oct 28, 2015 at 3:13 PM, Rob Clark <robdclark at gmail.com> wrote:
> 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..

Yup.  I'll change things around to take a nir_shader **.  Should be
easy enough to do.