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 CallC IR #8880

Merged
merged 8 commits into from May 27, 2020
Merged

[mypyc]Implement CallC IR #8880

merged 8 commits into from May 27, 2020

Conversation

TH3CHARLie
Copy link
Collaborator

@TH3CHARLie TH3CHARLie commented May 24, 2020

relates mypyc/mypyc#709

This PR adds a new IR op CallC to replace some PrimitiveOp that simply calls a C function. To demonstrate this prototype, str.join primitive is now switched from PrimitiveOp to CallC, with identical generated C code:

Test driver code

from typing import List

def test_str_join_helper(helper_arg: List[str]) -> str:
    helper_base = "#"
    return helper_base.join(helper_arg)

def main():
    l = ["a", "bb", "cc"]
    print(test_str_join_helper(l))

main()

generated C code(for the helper function, identical before/after):

PyObject *CPyDef_test_str_join_helper(PyObject *cpy_r_helper_arg) {
    PyObject *cpy_r_r0;
    PyObject *cpy_r_helper_base;
    PyObject *cpy_r_r1;
    PyObject *cpy_r_r2;
CPyL0: ;
    cpy_r_r0 = CPyStatic_unicode_3; /* '#' */
    CPy_INCREF(cpy_r_r0);
    cpy_r_helper_base = cpy_r_r0;
    cpy_r_r1 = PyUnicode_Join(cpy_r_helper_base, cpy_r_helper_arg);
    CPy_DecRef(cpy_r_helper_base);
    if (unlikely(cpy_r_r1 == NULL)) {
        CPy_AddTraceback("foo.py", "test_str_join_helper", 5, CPyStatic_globals);
        goto CPyL2;
    } else
        goto CPyL1;
CPyL1: ;
    return cpy_r_r1;
CPyL2: ;
    cpy_r_r2 = NULL;
    return cpy_r_r2;
}

Some objectives need to be completed after some discussion.

  • figure out the differences betweenOpDescrption and new LLOpDescrption, do we need fields including is_var_arg, steals, is_borrowed?(Not supported in this PR yet, but we will gradually update the design when we encounter ops that require such changes)
  • support void functions(through a flag?)
  • textual IR testcase

@TH3CHARLie TH3CHARLie changed the title Implement CFunctionCall IR [mypyc]Implement CFunctionCall IR May 24, 2020
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.

Overall looks good! Left some comments, mostly about naming. Naming is one of the most important factors that affects the readability of code, so it makes sense to think carefully about it. (It's also okay to later rename things if we come up with better names.)

mypyc/ir/ops.py Outdated Show resolved Hide resolved
mypyc/ir/ops.py Outdated Show resolved Hide resolved
mypyc/primitives/registry.py Outdated Show resolved Hide resolved
mypyc/ir/ops.py Outdated Show resolved Hide resolved

from mypyc.ir.ops import (
OpDescription, EmitterInterface, EmitCallback, StealsDescription, short_name
)
from mypyc.ir.rtypes import RType, bool_rprimitive

# TODO: comment, we don't need error_kind since C functions are all ERR_MAGIC
# however, other fields may need further investigation
LLOpDescription = NamedTuple(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This name doesn't fit the current implementation. Can you propose a new name that communicates that this specifically describes how to call a C function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am using CFunctionDescription in the updated commits.

@TH3CHARLie TH3CHARLie requested a review from JukkaL May 26, 2020 12:03
@TH3CHARLie TH3CHARLie changed the title [mypyc]Implement CFunctionCall IR [mypyc]Implement CallC IR May 26, 2020
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 updates! Just a few more naming ideas and comment about a comment, otherwise looks great.

mypyc/primitives/registry.py Outdated Show resolved Hide resolved
mypyc/primitives/registry.py Outdated Show resolved Hide resolved
mypyc/primitives/registry.py Outdated Show resolved Hide resolved
@TH3CHARLie TH3CHARLie requested a review from JukkaL May 26, 2020 17:25
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 great now!

@JukkaL JukkaL merged commit f94fc7e into python:master May 27, 2020
@TH3CHARLie TH3CHARLie deleted the mypyc-ir-cfunctioncall branch May 27, 2020 10:59
@TH3CHARLie
Copy link
Collaborator Author

Thanks for the review!

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.

None yet

2 participants