-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-98831: Refactor and fix cases generator #99526
Conversation
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.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A few comments:
PyObject *container = SECOND(); | ||
_PyBinarySubscrCache *cache = (_PyBinarySubscrCache *)next_instr; | ||
uint32_t type_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super cool to see the cache reads in action!
Python/bytecodes.c
Outdated
@@ -3197,7 +3184,7 @@ dummy_func( | |||
goto error; | |||
} | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to change this (also the two instances below)?
@@ -90,13 +89,12 @@ def name(self) -> str: | |||
return self.header.name | |||
|
|||
@property | |||
def inputs(self) -> list[Effect]: | |||
def inputs(self) -> list[InputEffect]: | |||
return self.header.inputs | |||
|
|||
@property | |||
def outputs(self) -> list[StackEffect]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def outputs(self) -> list[StackEffect]: | |
def outputs(self) -> list[OutputEffect]: |
if ceffect.name != "unused": | ||
# TODO: if name is 'descr' use PyObject *descr = read_obj(...) | ||
bits = ceffect.size * 16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.name for seffect in self.input_effects] | ||
for i, output in enumerate(reversed(self.output_effects), 1): | ||
if output.name not in input_names and output.name != "unused": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
while tkn := psr.next(raw=True): | ||
if tkn.text == BEGIN_MARKER: | ||
break | ||
else: | ||
raise psr.make_syntax_error(f"Unexpected token") | ||
return instrs, supers, families | ||
raise psr.make_syntax_error(f"Couldn't find {BEGIN_MARKER!r} in {psr.filename}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do this move in lots of places... maybe add a method?
def error(self, e: str) -> None:
print(e, file=sys.stderr)
self.errors += 1
if len(members) < 2: | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an error, yeah?
This refactor also fixes code generation for cache effects that aren't unused and removes
check_overlaps()
. It shows it all off for theBINARY_SUBSCR
family. (I had an earlier PR (GH-99408) for this, but it got a bit stale and I decided to delete the history.)