Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
markshannon commented Feb 16, 2024 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner commented Feb 16, 2024
Also, I like how clean this is in the DSL! |
markshannon commented Feb 16, 2024
| elseif (oparg<_PyUop_Replication[opcode]){ | ||
| buffer[pc].opcode=opcode+oparg+1; | ||
| } | ||
| elseif (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.)
Fidget-Spinner left a comment
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.
| properties=compute_properties(op), | ||
| ) | ||
| ifeffect_depends_on_oparg_1(op) and"split"inop.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?
Fidget-SpinnerFeb 19, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
gvanrossum left a comment
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!
| elseif (oparg<_PyUop_Replication[opcode]){ | ||
| buffer[pc].opcode=opcode+oparg+1; | ||
| } | ||
| elseif (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.)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
encukou commented Feb 20, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Adds the
splitandreplicate(N)annotationssplitsplits 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.