<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Mar 17, 2018 at 10:32 PM, Dylan Baker <span dir="ltr"><<a href="mailto:dylan@pnwbakers.com" target="_blank">dylan@pnwbakers.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Quoting Jason Ekstrand (2018-03-17 09:53:13)<br>
<div><div class="h5">> On March 16, 2018 23:36:50 Dylan Baker <<a href="mailto:dylan@pnwbakers.com">dylan@pnwbakers.com</a>> wrote:<br>
><br>
> > Quoting Rob Clark (2018-03-16 14:25:09)<br>
> >> On Fri, Mar 16, 2018 at 4:30 PM, Dylan Baker <<a href="mailto:dylan@pnwbakers.com">dylan@pnwbakers.com</a>> wrote:<br>
> >> > Quoting Rob Clark (2018-03-16 12:20:10)<br>
> >> >> On Fri, Mar 16, 2018 at 3:13 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br>
> >> wrote:<br>
> >> >> > On Fri, Mar 16, 2018 at 11:53 AM, Dylan Baker <<a href="mailto:dylan@pnwbakers.com">dylan@pnwbakers.com</a>> wrote:<br>
> >> >> >><br>
> >> >> >> Quoting Jason Ekstrand (2018-03-16 11:38:47)<br>
> >> >> >> > On Fri, Mar 16, 2018 at 11:28 AM, Dylan Baker <<a href="mailto:dylan@pnwbakers.com">dylan@pnwbakers.com</a>><br>
> >> >> >> > wrote:<br>
> >> >> >> ><br>
> >> >> >> > intr_opcodes = {<br>
> >> >> >> > 'nop': Intrinsic('nop', flags=[CAN_ELIMINATE]),<br>
> >> >> >> > ...<br>
> >> >> >> > }<br>
> >> >> >> ><br>
> >> >> >> > I prefer this since each dictionary is clearly created without a<br>
> >> >> >> > function<br>
> >> >> >> > obscuring what's actually going on. If you dislike having to repeat<br>
> >> >> >> > the<br>
> >> >> >> > name you<br>
> >> >> >> > could even do something like:<br>
> >> >> >> > intr_opcodes = [<br>
> >> >> >> > 'nop': Intrinsic('nop', flags=[CAN_ELIMINATE]),<br>
> >> >> >> > ...<br>
> >> >> >> > ]<br>
> >> >> >> > intr_opcodes = {<a href="http://i.name" rel="noreferrer" target="_blank">i.name</a>: i for i in intr_opcodes}<br>
> >> >> >> ><br>
> >> >> >> ><br>
> >> >> >> > I'm not sure what I think about this. On the one hand, having the<br>
> >> >> >> > dictionary<br>
> >> >> >> > explicitly declared is nice. On the other hand, in nir_opcodes.py we<br>
> >> >> >> > have a<br>
> >> >> >> > bunch of other helper functions we declare along the way to help with<br>
> >> >> >> > specific<br>
> >> >> >> > kinds of opcodes. It's not as practical to do this if everything is<br>
> >> >> >> > inside of<br>
> >> >> >> > a dictionary declaration.<br>
> >> >> >><br>
> >> >> >> Why not?<br>
> >> >> >><br>
> >> >> >> def make_op(name, *args):<br>
> >> >> >> return Intrinsic(name, foo='bar', *args)<br>
> >> >> >><br>
> >> >> >> intr_opcodes = [<br>
> >> >> >> make_op('nop', ...),<br>
> >> >> >> ]<br>
> >> >> ><br>
> >> >> ><br>
> >> >> > Because it's nice to keep the definition of the wrapper close to where<br>
> >> it's<br>
> >> >> > used.<br>
> >> >> ><br>
> >> >><br>
> >> >> also, fwiw, I end up needing two sets (or possibly lists), a second<br>
> >> >> one for the builders that are generated for sysval intrinsics.. I'm<br>
> >> >> not entirely sure how that would work if everything was declared<br>
> >> >> inline instead of via helper fxns<br>
> >> ><br>
> >> > I'm missing where a helper function adds an intrinsic to more than one list.<br>
> >><br>
> >> system_value() adds to system_values table, after calling intrinsic()<br>
> >> which adds to the global table (this is the reason for the return<br>
> >> statement in intrinsic()<br>
> >><br>
> >> BR,<br>
> >> -R<br>
> >><br>
> ><br>
> > I don't know, maybe it's just that I really don't like side effects, but<br>
> > functions with side-effects that call functions with side-effects make me<br>
> > nervous.<br>
><br>
> I think side effects are ok here. This is one of those cases where we're<br>
> not so much writing python code as writing a tiny DSL in python for<br>
> declaring NIR intrinsics. Alternatively, you can think of the "intrinsic"<br>
> function as a sort of "add this to a database" function which is something<br>
> which has fairly natural and easy to understand side effects. In either<br>
> case, the focus needs to be on the NIR intrinsics being declared and the<br>
> details of how the python works should fade into the background.<br>
><br>
<br>
</div></div>That's the sort of logic that made the mess in mapi/glapi/gen. These generators<br>
are like the shell script thats "just a one off script, it's just a few lines",<br>
and suddenly ten people have modified it and no one understands how it works or<br>
can modify it without everything falling apart.</blockquote><div><br>That is likely true. :-) <br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Code generators need to be clean<br>
and maintainable just like the C and C++ code that makes up mesa. Seriously,<br>
this is why I'm rewriting all of the generators in mapi/glapi/gen instead of<br>
trying to fix them to work with Khronos XML.<span class="HOEnZb"></span><br></blockquote><div><br></div><div>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.<br><br>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:<br><br></div><div> 1) Duplicating names of opcodes so that they can get out-of-sync<br></div><div> 2) Replacing immutable objects with mutable ones (so we can add names later)<br></div><div> 3) Expose the underlying data structure used to store the opcodes.<br><br></div><div>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.<br></div><div><br></div><div>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.<br><br></div><div>--Jason<br></div></div></div></div>