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

[mypyc] Implement builtins.len primitive for list #9271

Merged
merged 9 commits into from
Aug 7, 2020
Merged

[mypyc] Implement builtins.len primitive for list #9271

merged 9 commits into from
Aug 7, 2020

Conversation

TH3CHARLie
Copy link
Collaborator

@TH3CHARLie TH3CHARLie commented Aug 5, 2020

In this PR,

  • pointer_rprimitive is introduced to represent pointer type
  • PyVarObject is registerd
  • size_t_to_short_int custom op is added
  • GetElementPtr's codegen is largely changed.

I am marking this as a draft now and I expect tests to fail. Tests will be fixed once we decide the final design.

@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented Aug 5, 2020

for this function:

def f(a: List[int]) -> int:
    return len(a)

originally:

CPyTagged CPyDef_f(PyObject *cpy_r_a) {
    CPyTagged cpy_r_r0;
    Py_ssize_t __tmp1;
CPyL0: ;
    __tmp1 = PyList_GET_SIZE(cpy_r_a);
    cpy_r_r0 = CPyTagged_ShortFromSsize_t(__tmp1);
    return cpy_r_r0;
}

now:

CPyTagged CPyDef_f(PyObject *cpy_r_a) {
    CPyPtr cpy_r_r0;
    int64_t cpy_r_r1;
    CPyTagged cpy_r_r2;
CPyL0: ;
    cpy_r_r0 = (CPyPtr)&((PyVarObject *)cpy_r_a)->ob_size;
    cpy_r_r1 = *(int64_t *)cpy_r_r0;
    cpy_r_r2 = CPyTagged_ShortFromSsize_t(cpy_r_r1);
    return cpy_r_r2;
}

IR:

L0:
    r0 = get_element_ptr a ob_size :: PyVarObject
    r1 = load_mem r0 :: int64*
    r2 = CPyTagged_ShortFromSsize_t(r1)
    return r2

@TH3CHARLie TH3CHARLie marked this pull request as draft August 5, 2020 17:55
@TH3CHARLie TH3CHARLie requested a review from JukkaL August 5, 2020 17:57
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.

Here's my first quick review pass (not a full review).

mypyc/ir/rtypes.py Outdated Show resolved Hide resolved
mypyc/irbuild/specialize.py Outdated Show resolved Hide resolved
mypyc/primitives/struct_regsitry.py Outdated Show resolved Hide resolved
@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented Aug 6, 2020

@JukkaL I find a potential problem when fixing the IR tests: although we implement builtins.len for list in the specializers, list_len_op is exported and used in some places directly. We should overcome that as well. I am thinking about adding a list_len to LowLevelBuilder so that both specializer and other places can use it. What do you think?

@TH3CHARLie TH3CHARLie marked this pull request as ready for review August 6, 2020 13:29
@TH3CHARLie TH3CHARLie requested a review from JukkaL August 6, 2020 13:29
@JukkaL
Copy link
Collaborator

JukkaL commented Aug 6, 2020

I am thinking about adding a list_len to LowLevelBuilder so that both specializer and other places can use it. What do you think?

That's a good idea.

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 the updates! Looks mostly good, but there's one call op that I want to get rid of.

mypyc/ir/rtypes.py Outdated Show resolved Hide resolved
mypyc/ir/rtypes.py Outdated Show resolved Hide resolved
mypyc/irbuild/specialize.py Outdated Show resolved Hide resolved
mypyc/primitives/int_ops.py Outdated Show resolved Hide resolved
@TH3CHARLie TH3CHARLie requested a review from JukkaL August 6, 2020 19:09
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, looks good now! I left one comment -- feel free to merge this yourself once you've addressed it.

mypyc/primitives/int_ops.py Outdated Show resolved Hide resolved
@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented Aug 7, 2020

I just updated that, I'll merge this once all tests go green.

@TH3CHARLie TH3CHARLie merged commit 68763ae into python:master Aug 7, 2020
@TH3CHARLie TH3CHARLie deleted the list-len branch August 7, 2020 10:26
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