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 float abs primitive #9695

Merged
merged 4 commits into from
Nov 4, 2020
Merged

[mypyc] Implement float abs primitive #9695

merged 4 commits into from
Nov 4, 2020

Conversation

TH3CHARLie
Copy link
Collaborator

related mypyc/mypyc#644, also part of the plan to speed up rest of the slow microbenmark ops

Implement builtins.abs on float.

@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented Nov 3, 2020

on float_abs:

before:

interpreted: 0.434341s (avg of 5 iterations; stdev 1.5%)
compiled:    0.423638s (avg of 5 iterations; stdev 2.3%)

compiled is 1.025x faster

after:

interpreted: 0.422043s (avg of 5 iterations; stdev 1%)
compiled:    0.145499s (avg of 5 iterations; stdev 1.9%)

compiled is 2.901x faster

I'd like to add a run test for this, but looks like abs lacks in the testing shed

@TH3CHARLie TH3CHARLie requested a review from JukkaL November 3, 2020 12:01
@JukkaL
Copy link
Collaborator

JukkaL commented Nov 4, 2020

I'd like to add a run test for this, but looks like abs lacks in the testing shed

You can add abs to mypyc/test-data/fixtures/ir.py. Since SupportsAbs is not available, it's fine to use an overloaded function (int -> int and float -> float) instead.

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, but it would be nice to have a test case for this (see my other comment).

mypyc/test-data/run-floats.test Outdated Show resolved Hide resolved

[case testFloatAbs]
[file driver.py]
assert abs(0) == 0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm the result of abs(0) should be of type int. Can you check that the return value is not a float here? You may need to add a similar primitive for abs(integer).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line is incorrect, it should be abs(0.0) == 0.0 while the test will pass because it gets interpreted and 0 == 0.0 returns True. It should be fixed now.

I am not getting what do you mean by adding a primitive for integer? Do you mean a fixture function or a mypyc primitive?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean a primitive for abs(5), where the argument is an integer (in int_ops.py). It can use PyNumber_Absolute for now, similar to the float op, but the return type would be int instead of float.

I think that we may currently use the float variant of the primitive for integer arguments, which doesn't seem right. More generally, it's better for consistency to also cover the int case.

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 see. I've verified that in the above case, abs(0) returns an int by checking the generated code and running the compiled module. So we wouldn't be using float variant for integer arguments.

The integer version primitive would better be implemented in a separate PR as it requires casts from CPyTagged to PyObject * if we are going to reuse PyNumber_Absolute

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing this in a separate PR sounds good. I was worried that we might use the float op with an int argument since int is compatible with float. Thanks for checking that this isn't a real issue.

Implementing the integer variant in IR build would be even better, since it would avoid boxing/unboxing. Can you create a follow-up issue about this (or PR)? However, using PyNumber_Absolute would be a reasonable intermediate step.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are on the same track. By building the op in irbuild, we can have a fast path for short ints.

@JukkaL JukkaL merged commit 613c0ce into python:master Nov 4, 2020
@TH3CHARLie TH3CHARLie deleted the float-abs branch November 4, 2020 17:44
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