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

[Dylan Baker]

Quoting Jason Ekstrand (2018-03-17 23:37:51)
> 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

The side effects of side effects is *exactly* why the GLX generators are
*HORRIBLE*, you can't change anything because every function has a return value
and at least one side-effect. Want to alter a return value? oh, sorry that
causes a function three levels down in the call chain's side effect to change
and now the generated code is completely broken. At the very least I will put my
foot down hard on return values and side effects. I really don't like side
effects, but pick one. And you're right, I'm asking that the python code that
has to be maintained should be maintainable. I don't understand why that's such
a big ask.

Also, this is python there is not concept of immutability, and we're
not treating anything as immutable inside of the python file anyway, all of the
functions mutate the collections of intrinsics.

I'll do the conversion myself and send it to the list.

Dylan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180319/a4840ce2/attachment-0001.sig>