Skip to content

[mypyc] Generates inline compare statement on short ints and fix sieve performance regression #9127

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 13, 2020
Merged

[mypyc] Generates inline compare statement on short ints and fix sieve performance regression #9127

merged 5 commits into from
Jul 13, 2020

Conversation

TH3CHARLie
Copy link
Collaborator

@TH3CHARLie TH3CHARLie commented Jul 11, 2020 •
edited
Loading

relates mypyc/mypyc#750

Generate almost identical code as before be01236

before:

CPyL1: ;
    cpy_r_r4 = (Py_ssize_t)cpy_r_r3 < (Py_ssize_t)cpy_r_r2;
    if (cpy_r_r4) {
        goto CPyL2;
    } else
        goto CPyL5;

now with this PR:

CPyL1: ;
    cpy_r_r4 = cpy_r_r3 < cpy_r_r2;
    if (cpy_r_r4) {
        goto CPyL2;
    } else
        goto CPyL5;

@TH3CHARLie TH3CHARLie changed the title [mypyc] Generates inline compare statement on short ints and fix sieve regression [mypyc] Generates inline compare statement on short ints and fix sieve performance regression Jul 11, 2020
@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented Jul 11, 2020 •
edited
Loading

IR test changes are a bit of messy because the integer < got expanded. To see the change in list for, check irbuild-lists and irbuild-statements.

This should also fix the int_lists regression as well.

JukkaL
JukkaL reviewed Jul 13, 2020
View reviewed changes
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the regression! We'll see whether it's all gone after the nightly benchmark runs. Looks good, just a few minor comments.

mypyc/primitives/int_ops.py Outdated
int_less_than_ = c_custom_op(
arg_types=[int_rprimitive, int_rprimitive],
return_type=bool_rprimitive,
c_function_name='CPyTagged_IsLt',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have a _ suffix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it definitely should! Thanks for catching this potentially costly bug!

@TH3CHARLie TH3CHARLie requested a review from JukkaL July 13, 2020 13:04
@JukkaL JukkaL merged commit 358522e into python:master Jul 13, 2020
@TH3CHARLie TH3CHARLie mentioned this pull request Jul 22, 2020
Merged
@TH3CHARLie TH3CHARLie deleted the fix-sieve-regression branch July 22, 2020 10:39
JukkaL pushed a commit that referenced this pull request Jul 22, 2020
@TH3CHARLie
[mypyc] Expand int ops when operation contains at least one tagged int (
32a6377 
#9187)

Follow-up of #9108 and #9127, generates the new inlined style ops when there is at 
least one tagged integer present (the previous two PRs actually specialized two ints 
and two short_ints cases). After this and the remaining merge, we will get rid of 
`CPyTagged_IsEq`, `CPyTagged_IsNe`, `CPyTagged_IsLt`, `CPyTagged_IsLe`, 
`CPyTagged_IsGt` and `CPyTagged_IsGe`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JukkaL JukkaL JukkaL approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TH3CHARLie @JukkaL