[Mesa-dev] [PATCH v0] nir: mako all the intrinsics
Jason Ekstrand
jason at jlekstrand.net
Mon Mar 19 22:53:52 UTC 2018
On Mon, Mar 19, 2018 at 10:21 AM, Dylan Baker <dylan at pnwbakers.com> wrote:
> 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 think I agree with you on return values. If we're going to use a
"register an intrinsic" model, we shouldn't have magic
create+register+other functions. We need to pick and I'd like to pick
side-effects and can return values.
> 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.
>
I also want it to be maintainable. I just don't think that
INTRINSICS = {
'foo' = intrinsic('foo', ...)
...
}
is maintainable.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180319/b7f1573c/attachment-0001.html>
More information about the mesa-dev
mailing list