[Mesa-dev] [PATCH v0] nir: mako all the intrinsics

[Jason Ekstrand]

On Sat, Mar 17, 2018 at 10:32 PM, Dylan Baker <dylan at pnwbakers.com> wrote:

> Quoting Jason Ekstrand (2018-03-17 09:53:13)
> > On March 16, 2018 23:36:50 Dylan Baker <dylan at pnwbakers.com> wrote:
> >
> > > Quoting Rob Clark (2018-03-16 14:25:09)
> > >> On Fri, Mar 16, 2018 at 4:30 PM, Dylan Baker <dylan at pnwbakers.com>
> wrote:
> > >> > Quoting Rob Clark (2018-03-16 12:20:10)
> > >> >> On Fri, Mar 16, 2018 at 3:13 PM, Jason Ekstrand <
> jason at jlekstrand.net>
> > >> wrote:
> > >> >> > On Fri, Mar 16, 2018 at 11:53 AM, Dylan Baker <
> dylan at pnwbakers.com> wrote:
> > >> >> >>
> > >> >> >> Quoting Jason Ekstrand (2018-03-16 11:38:47)
> > >> >> >> > On Fri, Mar 16, 2018 at 11:28 AM, Dylan Baker <
> dylan at pnwbakers.com>
> > >> >> >> > wrote:
> > >> >> >> >
> > >> >> >> >     intr_opcodes = {
> > >> >> >> >         'nop': Intrinsic('nop', flags=[CAN_ELIMINATE]),
> > >> >> >> >         ...
> > >> >> >> >     }
> > >> >> >> >
> > >> >> >> >     I prefer this since each dictionary is clearly created
> without a
> > >> >> >> > function
> > >> >> >> >     obscuring what's actually going on. If you dislike having
> to repeat
> > >> >> >> > the
> > >> >> >> >     name you
> > >> >> >> >     could even do something like:
> > >> >> >> >     intr_opcodes = [
> > >> >> >> >         'nop': Intrinsic('nop', flags=[CAN_ELIMINATE]),
> > >> >> >> >         ...
> > >> >> >> >     ]
> > >> >> >> >     intr_opcodes = {i.name: i for i in intr_opcodes}
> > >> >> >> >
> > >> >> >> >
> > >> >> >> > I'm not sure what I think about this.  On the one hand,
> having the
> > >> >> >> > dictionary
> > >> >> >> > explicitly declared is nice.  On the other hand, in
> nir_opcodes.py we
> > >> >> >> > have a
> > >> >> >> > bunch of other helper functions we declare along the way to
> help with
> > >> >> >> > specific
> > >> >> >> > kinds of opcodes.  It's not as practical to do this if
> everything is
> > >> >> >> > inside of
> > >> >> >> > a dictionary declaration.
> > >> >> >>
> > >> >> >> Why not?
> > >> >> >>
> > >> >> >> def make_op(name, *args):
> > >> >> >>     return Intrinsic(name, foo='bar', *args)
> > >> >> >>
> > >> >> >> intr_opcodes = [
> > >> >> >>     make_op('nop', ...),
> > >> >> >> ]
> > >> >> >
> > >> >> >
> > >> >> > Because it's nice to keep the definition of the wrapper close to
> where
> > >> it's
> > >> >> > used.
> > >> >> >
> > >> >>
> > >> >> also, fwiw, I end up needing two sets (or possibly lists), a second
> > >> >> one for the builders that are generated for sysval intrinsics.. I'm
> > >> >> not entirely sure how that would work if everything was declared
> > >> >> inline instead of via helper fxns
> > >> >
> > >> > I'm missing where a helper function adds an intrinsic to more than
> one list.
> > >>
> > >> system_value() adds to system_values table, after calling intrinsic()
> > >> which adds to the global table (this is the reason for the return
> > >> statement in intrinsic()
> > >>
> > >> BR,
> > >> -R
> > >>
> > >
> > > I don't know, maybe it's just that I really don't like side effects,
> but
> > > functions with side-effects that call functions with side-effects make
> me
> > > nervous.
> >
> > I think side effects are ok here.  This is one of those cases where we're
> > not so much writing python code as writing a tiny DSL in python for
> > declaring NIR intrinsics.  Alternatively, you can think of the
> "intrinsic"
> > function as a sort of "add this to a database" function which is
> something
> > which has fairly natural and easy to understand side effects.  In either
> > case, the focus needs to be on the NIR intrinsics being declared and the
> > details of how the python works should fade into the background.
> >
>
> That's the sort of logic that made the mess in mapi/glapi/gen. These
> generators
> are like the shell script thats "just a one off script, it's just a few
> lines",
> and suddenly ten people have modified it and no one understands how it
> works or
> can modify it without everything falling apart.


That is likely true. :-)

Code generators need to be clean
> and maintainable just like the C and C++ code that makes up mesa.
> Seriously,
> this is why I'm rewriting all of the generators in mapi/glapi/gen instead
> of
> trying to fix them to work with Khronos XML.
>

Yes they do and I whole-heatedly agree with your desire for cleanliness.
The reason why this particular script gets sticky is that there are two
pieces whose cleanliness is in conflict: the list of intrinsic definitions,
and the python code which puts them into a data structure.  You are arguing
for the cleanliness of the python code over the list of declarations.  What
is really important in this file, however, is the list of declarations.
The python code that turns those declarations into a data structure is just
an implementation detail.

Unfortunately, all of the mechanisms that have so-far been suggested to
clean up the python have one or more of the following side-effects:

 1) Duplicating names of opcodes so that they can get out-of-sync
 2) Replacing immutable objects with mutable ones (so we can add names
later)
 3) Expose the underlying data structure used to store the opcodes.

The third one is interesting because, in nir_opcodes.py, I believe we did
actually change the underlying data structure at one point.  If it's
exposed, then doing so requires altering every single line of the file.

The more I think about this, the more I think that Rob's code is
more-or-less the right direction.  I'm not sure what to think about system
values and agree that adding a side-effecting wrapper for those seems
wrong.  However, I think that having declaration functions is, in this
case, actually the cleanest way to do it when you consider all of the
different pieces in play.

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180317/bf15eac0/attachment-0001.html>