Skip to content

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

Merged
TH3CHARLie merged 9 commits intopython:masterfrom
TH3CHARLie:list-len
Aug 7, 2020
Merged

[mypyc] Implement builtins.len primitive for list#9271
TH3CHARLie merged 9 commits intopython:masterfrom
TH3CHARLie:list-len

Conversation

@TH3CHARLie
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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).

@TH3CHARLie
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@TH3CHARLie TH3CHARLie requested a review from JukkaL August 6, 2020 19:09
Copy link
Copy Markdown
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.

@TH3CHARLie
Copy link
Copy Markdown
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