-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
GH-115457: Support splitting and replication of micro ops. #115558
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
Conversation
Also, I like how clean this is in the DSL! |
else if (oparg < _PyUop_Replication[opcode]) { | ||
buffer[pc].opcode = opcode + oparg + 1; | ||
} | ||
else if (opcode == _JUMP_TO_TOP || opcode == _EXIT_TRACE) { |
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.
Maybe use op_is_end
here?
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.
It's a static function in another file.
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.
We'll leave it up to Ken Jin to turn it into a macro or static inline in a header. (Because he's adding another opcode that could end the list of opcodes, a new JUMP variant.)
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 in general, just one minor comment.
@@ -483,6 +505,49 @@ def make_uop(name: str, op: parser.InstDef, inputs: list[parser.InputEffect]) -> | |||
body=op.block.tokens, | |||
properties=compute_properties(op), | |||
) | |||
if effect_depends_on_oparg_1(op) and "split" in op.annotations: | |||
result.properties.oparg_and_1 = True |
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 code consistency, shouldn't this be compute_properties
?
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.
Shouldn't what be compute_properties
?
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.
Woops I meant shouldn't this be computed in compute_properties
?
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.
The oparg_and_1
property only applies to the base uop, not the replicas as their behavior does not depend on oparg & 1
. compute_properties
computes the properties from the definition only, so we would need to modify oparg_and_1
anyway.
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.
Cool!
else if (oparg < _PyUop_Replication[opcode]) { | ||
buffer[pc].opcode = opcode + oparg + 1; | ||
} | ||
else if (opcode == _JUMP_TO_TOP || opcode == _EXIT_TRACE) { |
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.
We'll leave it up to Ken Jin to turn it into a macro or static inline in a header. (Because he's adding another opcode that could end the list of opcodes, a new JUMP variant.)
Adds the
split
andreplicate(N)
annotationssplit
splits uops into two depending on the low bit of theoparg
. This removes a few jumps from uops like_LOAD_ATTR_INSTANCE_VALUE
.replicate(N)
replicates the original uop for each oparg inrange(N)
. This is particularly valuable for uops that loop overoparg
, but it is also useful to inline the oparg at build time rather than when patching.