Skip to content

Conversation

TH3CHARLie
Copy link
Collaborator

related mypyc/mypyc#734

@TH3CHARLie TH3CHARLie marked this pull request as draft June 26, 2020 06:30
@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented Jun 26, 2020

There will be auto-generated Box, Unbox, DecRef sequence right after the CallC which I don't expect to have.
Screen Shot 2020-06-26 at 2 30 54 PM
in the following code's case, this sequence will try to check whether the return value from CallC is a boolean.

def foo(x: object) ->bool:
    z = isinstance(x, str)
    return z

Since int32_rprimitive is an unboxed type, this would lead to an assertion failure during emit.

update:
fixed

@TH3CHARLie
Copy link
Collaborator Author

With this PR, for the following python function:

def foo(x: object) -> bool:
    z = isinstance(x, str)
    return z

The C code before:

char CPyDef_foo(PyObject *cpy_r_x) {
    PyObject *cpy_r_r0;
    char cpy_r_r1;
    char cpy_r_z;
    char cpy_r_r2;
CPyL0: ;
    cpy_r_r0 = (PyObject *)&PyUnicode_Type;
    int __tmp1 = PyObject_IsInstance(cpy_r_x, cpy_r_r0);
    if (__tmp1 < 0)
        cpy_r_r1 = 2;
    else
        cpy_r_r1 = __tmp1;
    if (unlikely(cpy_r_r1 == 2)) {
        CPy_AddTraceback("../foo.py", "foo", 2, CPyStatic_globals);
        goto CPyL2;
    } else
        goto CPyL1;
CPyL1: ;
    cpy_r_z = cpy_r_r1;
    return cpy_r_z;
CPyL2: ;
    cpy_r_r2 = 2;
    return cpy_r_r2;
}

after:

char CPyDef_foo(PyObject *cpy_r_x) {
    PyObject *cpy_r_r0;
    int32_t cpy_r_r1;
    char cpy_r_r2;
    char cpy_r_z;
    char cpy_r_r3;
CPyL0: ;
    cpy_r_r0 = (PyObject *)&PyUnicode_Type;
    cpy_r_r1 = PyObject_IsInstance(cpy_r_x, cpy_r_r0);
    if (unlikely(cpy_r_r1 < 0)) {
        CPy_AddTraceback("../foo.py", "foo", 2, CPyStatic_globals);
        goto CPyL2;
    } else
        goto CPyL1;
CPyL1: ;
    cpy_r_r2 = cpy_r_r1;
    cpy_r_z = cpy_r_r2;
    return cpy_r_z;
CPyL2: ;
    cpy_r_r3 = 2;
    return cpy_r_r3;
}

We actually reduce the comparison by 1 even though we introduce a new op in its IR.

@TH3CHARLie TH3CHARLie marked this pull request as ready for review June 26, 2020 12:13
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.

Looks good! Left a few comments about documentation.

@TH3CHARLie TH3CHARLie requested a review from JukkaL June 26, 2020 16:07
@TH3CHARLie
Copy link
Collaborator Author

Once this is merged, I'd start to merge the remaining ops during the weekends.

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!

@JukkaL JukkaL merged commit 59f4914 into python:master Jun 26, 2020
@TH3CHARLie TH3CHARLie deleted the negative-int-emit branch June 26, 2020 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants