GH-98831: Refactor and fix cases generator#99526
Uh oh!
There was an error while loading. Please reload this page.
Conversation
gvanrossum commented Nov 16, 2022
I have a newer version of the same code in GH-99495 (which adds macros and ops) but that's not fully baked and I figured I'd release this in stages so it's easier on the reviewer. If you'd rather review the tooling in even larger chunks let me know. (I think the changes to the instruction definitions are pretty mild in this one.) |
brandtbucher left a comment
brandtbucher left a comment There was a problem hiding this comment.
Looks good. A few comments:
| PyObject*container=SECOND(); | ||
| _PyBinarySubscrCache*cache= (_PyBinarySubscrCache*)next_instr; | ||
| uint32_ttype_version=read_u32(cache->type_version); | ||
| inst(BINARY_SUBSCR_GETITEM, (container, sub, unused/1, type_version/2, func_version/1--unused)){ |
There was a problem hiding this comment.
Super cool to see the cache reads in action!
Python/bytecodes.c Outdated
| } | ||
| PyObject*arg=TOP(); | ||
| PyObject*res=_PyCFunction_TrampolineCall(cfunc, PyCFunction_GET_SELF(callable), arg); | ||
| PyObject*res=cfunc(PyCFunction_GET_SELF(callable), arg); |
There was a problem hiding this comment.
Did you mean to change this (also the two instances below)?
| returnself.header.inputs | ||
| @property | ||
| defoutputs(self) ->list[StackEffect]: |
There was a problem hiding this comment.
| defoutputs(self) ->list[StackEffect]: | |
| defoutputs(self) ->list[OutputEffect]: |
| ifceffect.name!="unused": | ||
| # TODO: if name is 'descr' use PyObject *descr = read_obj(...) | ||
| bits=ceffect.size*16 |
There was a problem hiding this comment.
I had suggestions on these from the other PR, not sure if you saw (I like _ more than unused, and both "unused" and 16 might benefit from constant names in this file).
Unless you just disagreed, in which case, feel free to ignore this. :)
| # Write output stack effect assignments | ||
| input_names= [seffect.nameforseffectinself.input_effects] | ||
| fori, outputinenumerate(reversed(self.output_effects), 1): | ||
| ifoutput.namenotininput_namesandoutput.name!="unused": |
There was a problem hiding this comment.
For output.name not in input_names, don't we need to check that they occur at the same (stack-adjusted) position, and write them out if not?
| whiletkn:=psr.next(raw=True): | ||
| iftkn.text==BEGIN_MARKER: | ||
| break | ||
| else: | ||
| raisepsr.make_syntax_error(f"Unexpected token") | ||
| returninstrs, supers, families | ||
| raisepsr.make_syntax_error(f"Couldn't find {BEGIN_MARKER!r} in {psr.filename}") |
There was a problem hiding this comment.
Wild, I've never seen while/else before...
| print( | ||
| f"Unknown instruction {target!r} predicted in {instr.name!r}", | ||
| file=sys.stderr, | ||
| ) | ||
| self.errors+=1 |
There was a problem hiding this comment.
You do this move in lots of places... maybe add a method?
deferror(self, e: str) ->None: print(e, file=sys.stderr) self.errors+=1| iflen(members) <2: | ||
| continue |
There was a problem hiding this comment.
This should be an error, yeah?
3 participants
This refactor also fixes code generation for cache effects that aren't unused and removes
check_overlaps(). It shows it all off for theBINARY_SUBSCRfamily. (I had an earlier PR (GH-99408) for this, but it got a bit stale and I decided to delete the history.)