Thursday, 2023-06-08

*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC08:00
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc08:00
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC08:43
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc08:45
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC08:50
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc08:50
*** ghostmansd <ghostmansd!~ghostmans@> has joined #libre-soc11:01
lkclghostmansd, ack :)11:08
lkclyeah i like the matcher concept too11:09
lkclyou probably worked out by now that the "depth" parameter was a known-calculable quantity.11:10
lkcl* db: depth=011:10
lkcl* instructions: depth=111:10
lkcl* records: depth=211:10
lkcl* record-fields: depth=311:10
lkcl* ...11:10
lkcland that can obviously be hard-coded into the.... the.... visitors.11:11
lkcl> Is the whole problem that you don't want visitors to be passed to Whatever.visit()11:13
lkcl(i'll do a full reply on the bugreport)11:13
lkclnot quite: they should indeed... *but not in a child-loop*11:14
lkclthe child-looping (full recursion) should be an *entirely* separate "thing", just like it is in python 2.7 ast.py11:15
lkcland just like in, there is a base-class (actually, calling it AST would do fine, but "InsnDB" or something is just as fine, it makes no odds)11:17
lkclthat *everything* - the entire Instruction Database - absolutely every single class - inherits from11:17
lkclthis allows the walker-function to perform the "fields()" introspection11:18
lkclyou have to have this:11:18
lkclclass Opcode:11:19
lkcl   ....11:19
lkcl    def visitor_walker_function(self):11:19
lkcl       for field in self.opcode_fields:11:20
lkcl         yield field11:20
lkclclass Record:11:20
lkcl   ....11:20
lkcl    def visitor_walker_function(self):11:20
lkcl        yield name11:20
lkcl    yield section11:20
lkcl     yield ppc11:20
lkcl      for field in self.fields:11:21
lkcl          yield field11:21
lkcl       yield mdwn11:21
lkcletc. etc.11:21
lkclthat is a *frickin* lot of work....11:21
lkcl.... *all* replaceable with *one* function that is only about 8 lines long.11:21
lkcl>  I don't want any of this "if isinstance" crap, at all.11:27
lkclplease don't make the mistake of thinking that use of "insinstance" is "crap", the alternatives are much worse: forcing the introduction of a hidden property/function/object and using "hasattr" instead, which is really fragile and counter-intuitive11:28
lkclusing "isinstance()" is pretty normal11:28
ghostmansd[m]I'm really tired of this.12:17
ghostmansd[m]Short reply on yield: it's not about walking. Please read how context manager works.12:18
ghostmansd[m]There can be no more yields than one.12:18
lkclok then we need to drop contextmanager12:18
ghostmansd[m]yield simply marks "before" and "after".12:18
lkclwhich insndb class has more than one child-to-be-walked?  it's.... Operand, isn't it?12:19
lkclthat has two things: static_operands and dynamic_operands, doesn't it?12:19
lkclthe "before" needs to be called *on static_operands* followed by an "after"12:20
lkcland then12:20
lkcl"before" needs to be called on dynamic_operands followed by a *separate* after12:20
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC12:20
lkclremember that this is complex enough that i can't completely grasp it entirely, so we both have to be very patient.12:21
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc12:21
lkclit's also worth getting right because we'll be basing pretty much the entirety of the code on it (!)12:21
lkcllet me think through what you wrote here
*** ghostmansd <ghostmansd!~ghostmans@> has quit IRC12:23
lkclin particular: how do you differentiate a field that is a str (or SelectableInt, or Enum)12:24
lkclsuch that the use of dataclasses.fields() in iter_child_nodes() can distinguish them?12:26
lkcliter_child_nodes _isn't_ the public API btw.  it's a private function.12:26
lkclok i'll write some more questions, i do appreciate i'm not replying to acknowledge what you're trying to say.12:28
ghostmansd[m]I suggest this:12:31
ghostmansd[m]1. Visitor just have stub __call__ method with "pass". But yes, I think this should be context manager, because it separates before/on/after.12:31
ghostmansd[m]2. I can make other fields be walkable, OK. Beware they are low-level.12:31
ghostmansd[m]Is it sufficient to finally stop it?12:31
lkclif users are not aware that they are low-level, they should not be writing code.  like a SQL developer, they *have* to know the structure of the TABLEs that they create, not just the data!12:32
ghostmansd[m]I want to start writing something practical.12:32
ghostmansd[m]No they don't need to be aware of how CSVs are layed out.12:32
ghostmansd[m]This is not some damn SQL. The analogy is incorrect.12:33
lkcltrue but they do need to know that db contains insns, insns contain operands, operands contain fields etc. etc.12:33
ghostmansd[m]Yes but they can have it in a good form. cf. class Extra.12:33
lkclif they don't know that, they should not be let loose on the code.12:34
ghostmansd[m]Why should the binutils hacker know about our CSVs?12:35
ghostmansd[m]In details, at least.12:35
lkclnot the CSVs, the classes and their inheritance / member structure12:35
ghostmansd[m]They will want to see the generator and could have a data nicely wrapped.12:35
ghostmansd[m]That's not what I'm talking about.12:35
ghostmansd[m]Again, see class Extra.12:35
ghostmansd[m]And then compare it to fields we have in Record right now.12:36
ghostmansd[m]Notice the difference.12:36
lkclgimme 1 sec...12:36
ghostmansd[m]And then ask yourself what's simpler to use.12:36
ghostmansd[m]Extra has all info incorporated.12:36
ghostmansd[m]And that's simple to use.12:36
ghostmansd[m]Yes I can do it on db init.12:36
* lkcl thinking...12:37
ghostmansd[m]But there's no task and budget for it. Nor time.12:37
ghostmansd[m]In fact, even this task lacks the budget.12:37
lkcli got totally confused12:37
lkclno it's ok, i'm really sorry, i misread12:37
ghostmansd[m]My point is, I can totally walk around fields via dataclasses.fields.12:37
lkclthe "yield node" had me totally confused12:38
ghostmansd[m]It's just that the data is way too level.12:38
ghostmansd[m]too low-level12:38
ghostmansd[m]yield is just a context switch12:38
ghostmansd[m]You keep thinking it's a walking, but it's not.12:39
lkclthere's two issues.  the Visitors (ExtrasVisitor) is perfect.  i confused the use of "yield", thinking it was still part of visitor-walking.12:39
lkclreally sorry - i'm with you now, and it's perfect.12:39
ghostmansd[m]That's fin312:39
ghostmansd[m]I was confused too when I saw the manager12:39
ghostmansd[m]But it's cool, especially for printing C structa12:40
ghostmansd[m]Sorry, typing from phone12:40
lkcli do that - just not with IRC :)12:40
ghostmansd[m]And printing these opening braces, then contents, then closing braces12:40
ghostmansd[m]So that's why I like context managers12:40
lkclit'll work really well, yes12:41
lkclpolish-notation you do the "yield node" before the prints12:41
lkclreverse-polish notation you do the "yield node" last12:41
lkcl(something like that)12:41
ghostmansd[m]everything before yield is __enter__12:41
ghostmansd[m]Everything later is __exit__12:42
lkcli was totally getting confused by thinking that "yield node" was the *database-walking* (IVisitable) rather than "actions" (IVisitor)12:43
lkclso sorry12:43
ghostmansd[m]That's why I asked you to read comments12:44
lkclok so this just leaves the recursive-walking, which is not something that is a public part of the API.  we *never* expect users to override it12:44
ghostmansd[m]I generally don't post the bullshit without giving at least minor context.12:44
lkcli am having difficulty *understanding*, it's not so much the reading :)12:45
ghostmansd[m]What do you mean by recursive walking here?12:45
ghostmansd[m]The hierarchy is linear.12:45
ghostmansd[m]We're not expected to visit some nodes again.12:45
lkclno definitely not!12:45
lkclrecursive-descent.  tree.  depth-first, breadth-first.12:46
lkcltrying to find the right words here12:46
lkclah. doh.12:46
lkcl(tree-hierarchy-walking is often done recursively, i used the wrong word, sorry).12:47
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC12:49
lkcli'll sort out budgets - it might not specifically be under this particular one, but increased elsewhere ok?12:51
lkclprogrammerjake, i meant: i saw a couple of days ago that you posted a "CI report" and tracked down the diff that caused the error.12:52
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc12:53
lkclexcept the openpowerbot wasn't running at the time so it wasn't recorded12:53
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC12:55
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc12:55
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC13:03
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc13:03
ghostmansd[m]Ok, so, summary13:03
ghostmansd[m]A simple visitor base class with one __call__ and node argument; the caller is expected to check instance13:04
ghostmansd[m]Next, for fields. These either need to be refactored, or we don't care that fields are low-level.13:06
lkcl> A simple visitor base class ...13:07
lkclyes. matching can be a pre-filter on the (now separate) tree-walk13:08
lkcl(or just use "if ==" for really simple ones)13:08
lkcl> Next, for fields....13:09
ghostmansd[m]There's no "pre-filter". There must be either something collected (records), or the filtering should be filtered upon visiting.13:09
ghostmansd[m]Or at least upon walking, if we pass filter to walker.13:09
lkclwell... what's nice about the tree-walk "yield" is that it is possible to do the filtering "on-the-fly"13:10
ghostmansd[m]Again with this isinstance?13:10
lkclit's not aabsolutely strictly necessary to store the entire "wanted" set in a temporary13:10
ghostmansd[m]Because nodes on walk can be of different types.13:11
lkcli'd expect some people to detect the node-type in the filter function, there13:11
lkclit's up to them what to choose13:11
ghostmansd[m]In walk(), it's the filter callable that must check for node types.13:12
ghostmansd[m]If this is the place you want it.13:12
lkclyes that works13:12
ghostmansd[m]Yes but ugly.13:12
ghostmansd[m]I'd rather expect somebody to collect the relevant level entries.13:12
ghostmansd[m]e.g. db.subnodes only13:12
ghostmansd[m]then filter them...13:13
lkclfor simple things i'd expect to just use the "if ==" trick13:13
ghostmansd[m]as they like, without special magic arguments13:13
ghostmansd[m]Again: this trick only works as long as you check instance type.13:13
lkclyes.  any info they need it should be passed in.13:13
ghostmansd[m]You cannot just take an arbitrary node and check for its name.13:13
lkclwell, you *can* have *two* visitor-functions (def Record(self, node) and def Operand(self, node))13:14
ghostmansd[m]We're walking in circles13:14
lkcland in Record() store node as self.__current_record = node13:14
ghostmansd[m]That's exactly what I did13:14
lkclthen use that in def Operand(...)13:15
ghostmansd[m]And you wrote that you want isinstance instead13:15
ghostmansd[m]You must be fucking kidding me13:15
ghostmansd[m]Please stop doing this13:15
lkclexcept that - and i apologise for this - that was when the visitor-walking was still part of the API13:15
ghostmansd[m]No walking is different from visiting13:16
lkclah rats i've used the wrong word "visitor-walking" there, haven't i.13:16
ghostmansd[m]Again: please finally read the code. The subnodes are separate. The visitor handlers are separate.13:16
ghostmansd[m]The code follows AST exactly, except for managers.13:18
ghostmansd[m]Ok, I'll post what I suggest, gimme some minutes.13:18
lkclExtra inherits from Node... Record inherits from Node...13:19
lkcloof i've been sitting down for too long.13:19
lkclplease allow me to continue off-IRC? i allocated some budget, am just upping it13:20
*** ghostmansd <ghostmansd!~ghostmans@> has joined #libre-soc13:28
*** ghostmansd <ghostmansd!~ghostmans@> has quit IRC13:31
*** ghostmansd <ghostmansd!~ghostmans@> has joined #libre-soc13:32
*** ghostmansd <ghostmansd!~ghostmans@> has quit IRC13:33
*** ghostmansd <ghostmansd!~ghostmans@> has joined #libre-soc13:33
ghostmansdlkcl, option 1:
ghostmansdoption 2:
ghostmansdbasically the same, but in option 2 we have a separate property for nodes, called subnodes. It allows to affect what's returned.13:35
ghostmansdFrom visitor only 1 thing is left: the fact it's callable and the __call__ is expected to be a context manager.13:36
ghostmansdFor 1094, the budget is unfair. This should be split between 3 people.13:37
ghostmansdJacob deserves some credit for his idea on context manager.13:37
ghostmansdThis depends a bit on whether binutils rewriting is part of this task, though. I guess yes, considering the budget.13:40
ghostmansdI think option 1 is better, because it's more generic. After all, if somebody want's a high-level stuff, they will call methods anyway.13:45
ghostmansd(option 1 lacks is instance, but you got the idea)13:45
ghostmansdActually I think option 2 is kinda better because not everything is a dataclass in insndb...13:46
ghostmansdOK let's kinda do hybrid. Thing that's a dataclass will have subnodes implemented in terms of dataclasses.fields.13:47
lkcl>  not everything is a dataclass in insndb14:20
lkcl(that's why the "isinstance(node, AST)" is there in python 2.7, for the detection.14:21
lkcl> Jacob deserves some credit for his idea14:22
lkcli just want to check if Node has a def subnodes(self): pass, i think it does, doesn't it... 1 sec...14:22
ghostmansd> done14:23
lkclah - yield from () - okaay14:23
ghostmansdnot done, you forgot another person :-)14:23
lkcli'm fine, i have plenty from the ISA grant14:24
lkclso always having a "def subnodes()" even when it returns (), you can skip the "if isinstance(node, Node):" thing14:24
lkclthat just leaves "non-dataclass" things, and... ehmm... ehmehmehm...14:25
ghostmansdcheck new commits :-)14:25
ghostmansdah wait, not there yet14:25
lkclehm... yes :)  17 hrs ago;a=summary14:26
lkcl"with visitor(node=node):" - this triggers the "begin/node/end" on the node, calling the visitor function14:30
lkcl(which will call def Record() or def Operand() etc.)14:30
lkclack, got it14:30
ghostmansdhm, something strange with commit name14:31
ghostmansdbut OK14:31
lkclpffh :)14:31
ghostmansdnote the Dataclass class, not yet used14:31
ghostmansdthis can be inherited by stuff like Record, Extra or other dataclass-based14:31
ghostmansdso they can have the same subnodes() method14:32
lkcllike it14:32
lkclholy cow this is powerful14:32
ghostmansdI cannot use the Dataclass yet, because some fields are not Node-based...14:33
ghostmansdand also are too low-level14:33
ghostmansdbut this is doable14:33
ghostmansdPerhaps we can just make all fields to be Nodes14:34
lkclso how would e.g. "class Instruction(str): ..." be handled?14:34
lkclyes that's the idea14:34
lkclexcept for str int Enum14:34
ghostmansdDon't look at local class Istruction in db14:34
ghostmansdit's for other purpose14:34
ghostmansd1 sec...14:34
lkclok maybe Enum can inherit from Node14:34
ghostmansdcf line 12614:35
lkclInstruction and SVP64Instruction aren't going to be part of walking i take it14:35
ghostmansdI should have called this class CLIInstructionArgument14:35
ghostmansdor like this14:35
lkclahh ok14:35
lkclthe Visitors are mentally short.14:36
lkcllet me find a db-class...14:37
ghostmansdDatabase currently yields all its subnodes as `yield from self`14:37
ghostmansdand `__iter__` is `yield from self.__db`14:37
ghostmansd1 sec14:37
lkclnot class Opcode, because it has value: Value and mask:Mask, i would expect those to inherit from Node...14:38
ghostmansdI think opcode should be inherited, yes14:38
lkclah! PPCRecord14:38
lkcl    comment: str14:38
lkcl    unofficial: bool = False14:39
ghostmansdmask and value perhaps too, not sure if we need to pass these too14:39
ghostmansdpass -> visit14:39
lkclhow will the visit() function spot that PPCRecord.comment is a str yet still "yield" it?14:39
ghostmansdbool is non-inheritable, and with enums there are... hm... complications14:39
ghostmansdThat's why there are custom subnodes for now in some dataclasses :-)14:40
ghostmansdbut with str/bool/whatever-else it's simple14:40
lkclindeed.  this is why pyomo has a list of "builtin" nodes14:40
ghostmansdwe can just have Nodes-based stuff there instead14:40
ghostmansdcomment: String14:42
ghostmansdclass String(str, Node): pass14:42
lkclif using hasattr(node, "subnodes") - or "subnodes = getattr(node, 'subnodes', None)" or try/catch14:42
ghostmansdyes, this can be the alternative14:42
lkclit seems to be pushing things a lot, just to avoid using hasattr/getattr/isinstance14:43
ghostmansdthe class-based approach has 1 pro: if we ever need something from generic Node class, this will be propagated to all descendants14:43
lkclbtw do you know the difference between "type(node) == Node" and "isinstance(node, 'Node')?14:43
ghostmansdcurrently we only have subnodes() method there, but perhaps there might be other stuff which quacks like duck14:44
lkcltype is *fast*14:44
ghostmansdyes, inehritance14:44
ghostmansdtype checks strictly14:44
ghostmansdand Node will take derived classes into account14:44
lkcli "knew" that but temporarily forgot it :)14:44
ghostmansdNode -> isinstance14:44
ghostmansdactually there's yet another twist: issubclass(type(instance), SomeType)14:45
lkclahhh. it works... but... blech :)14:45
lkcli was so pissed when imputil was removed from python 314:45
ghostmansdOK I think we can move towards making fields of insndb node-derived14:46
ghostmansdfine with you?14:46
lkclyes fantastic14:46
ghostmansdand then yield these fields despite their "is it high level enough"14:46
ghostmansdbecause, well14:46
ghostmansdif somebody needs high-level stuff14:46
ghostmansdthey catch the Record and call some of its methods14:46
ghostmansdand this is just visiting what we have in database14:47
lkcli love the empty subnodes thing.14:47
ghostmansdand in database we have exactly what we have in fields14:47
ghostmansdthis is 1:1 our CSVs and fields14:47
lkclbtw: icing on the cake....14:47
lkclthe visit() function *should* - in theory - by using dataclasses.fields() - work if you pass in the *class* instead of the *instance*!14:48
lkclif you do "visit(Record, RecordVisitorLayout)"14:49
lkclit should work!14:49
lkclbecause dataclasses.fields() takes either an instance or a class14:49
lkcland that way it is possible to print out the *structure* of the entire database with the appropriate LayoutVisitor14:50
lkcli can foresee that being useful for e.g. header files14:51
lkclto create structs in c, based on the database layout14:51
*** ghostmansd <ghostmansd!~ghostmans@> has quit IRC14:53
ghostmansd[m]Yes it works, but: no values to be got14:56
ghostmansd[m]Just the field types14:57
ghostmansd[m]But yes, I even used dataclasses.fields in some classmethod14:57
lkclok i need to get back to the spec - lots to do15:05
lkclincluding a rewrite of how PO9 is to be used15:05
ghostmansd[m]Ack, thank you for discussion!15:11
ghostmansd[m]This was a tough journey :-)15:12
*** ghostmansd <ghostmansd!~ghostmans@> has joined #libre-soc15:16
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC15:34
*** ghostmansd <ghostmansd!~ghostmans@> has quit IRC15:34
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc15:35
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC15:41
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc15:42
*** ghostmansd <ghostmansd!> has joined #libre-soc15:59
*** ghostmansd <ghostmansd!> has quit IRC16:08
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC16:22
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has joined #libre-soc16:23
*** ghostmansd <ghostmansd!> has joined #libre-soc17:02
*** ghostmansd[m] <ghostmansd[m]!~ghostmans@> has quit IRC17:07
lkcltell me about it... :)17:07
*** ghostmansd <ghostmansd!> has quit IRC17:07
*** ghostmansd[m] <ghostmansd[m]!> has joined #libre-soc17:07
lkclit will be worth it, though, it's incredibly powerful17:08
lkcland also easy to appreciate why people get it mixed up17:08
*** ghostmansd <ghostmansd!> has joined #libre-soc17:11
*** ghostmansd <ghostmansd!> has quit IRC17:16
*** ghostmansd <ghostmansd!> has joined #libre-soc17:21
*** ghostmansd <ghostmansd!> has quit IRC17:26
*** yambo <yambo!> has quit IRC17:57
programmerjakelkcl: it was recorded, since irclog is not run by openpowerbot:18:09
programmerjakenote i posted a commit range and ghostmansd did all the work of figuring out what he broke18:10
*** yambo <yambo!> has joined #libre-soc18:10
*** ghostmansd <ghostmansd!> has joined #libre-soc18:23
*** ghostmansd <ghostmansd!> has quit IRC18:28
*** ghostmansd <ghostmansd!> has joined #libre-soc18:33
*** ghostmansd <ghostmansd!> has quit IRC18:38
lkclprogrammerjake, ahh awesome. i added it
lkcland i think i know what's going on23:39

Generated by 2.17.1 by Marius Gedminas - find it at!