Skip to content
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-115457: Support splitting and replication of micro ops. #115558

Merged
merged 6 commits into from
Feb 20, 2024

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Feb 16, 2024

Adds the split and replicate(N) annotations

split splits uops into two depending on the low bit of the oparg. This removes a few jumps from uops like _LOAD_ATTR_INSTANCE_VALUE.

replicate(N) replicates the original uop for each oparg in range(N). This is particularly valuable for uops that loop over oparg, but it is also useful to inline the oparg at build time rather than when patching.

@Fidget-Spinner
Copy link
Member

Also, I like how clean this is in the DSL!

@markshannon
Copy link
Member Author

about 1% faster

else if (oparg < _PyUop_Replication[opcode]) {
buffer[pc].opcode = opcode + oparg + 1;
}
else if (opcode == _JUMP_TO_TOP || opcode == _EXIT_TRACE) {
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member

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.)

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a 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
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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 ?

Copy link
Member Author

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.

Copy link
Member

@gvanrossum gvanrossum left a 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) {
Copy link
Member

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.)

Python/executor_cases.c.h Show resolved Hide resolved
Tools/cases_generator/tier2_generator.py Show resolved Hide resolved
@markshannon markshannon merged commit 626c414 into python:main Feb 20, 2024
56 checks passed
@encukou
Copy link
Member

encukou commented Feb 20, 2024

The refleaks buildbot failed on test.test_capi.test_opt.TestUops.test_confidence_score. Likely causes: GH-114142, GH-115558, or GH-115688
I'll investigate later if it's not fixed; you probably have more context.

Edit: it's likely to be fixed in #115728

@markshannon markshannon deleted the split-on-oparg-1 branch February 21, 2024 16:19
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants