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

py/runtime: Remove unnecessary smallint handling. #3893

Closed

Conversation

Jongy
Copy link
Contributor

@Jongy Jongy commented Jun 24, 2018

This logic is done in mp_obj_new_int anyway. Removing this decreases
code size.

This logic is done in `mp_obj_new_int` anyway. Removing this decreases
code size.
@dpgeorge
Copy link
Member

I think the reason it was originally done the way it is is for performance: this is a very hot code path for small-int arithmetic (which accounts for a lot of the int arithmetic) so benefits from the small optimisation done here. If anything, the mp_obj_new_int() call could be replaced with mp_obj_new_int_from_ll(), which essentially does what the comment suggests, namely to inline the mp_obj_new_int() call.

@dpgeorge
Copy link
Member

If anything, the mp_obj_new_int() call could be replaced with mp_obj_new_int_from_ll()

I've now done that in e94d644 . So this PR can be closed, but thanks for prompting this nice change!

@dpgeorge dpgeorge closed this Jul 14, 2018
@Jongy
Copy link
Contributor Author

Jongy commented Jul 14, 2018

Hm. The code duplication remains with mp_obj_new_int, so maybe it could be defined as inline and be inlined here and in every objint implementation.

@dpgeorge
Copy link
Member

The code duplication remains with mp_obj_new_int, so maybe it could be defined as inline and be inlined here and in every objint implementation.

Yes it is duplicated. But it would likely increase code size by a lot if inlined unconditionally everywhere. The point with this particular case in runtime.c:mp_binary_op is that this is the primary code that is executed whenever you do any arithmetic on small integers. This is quite a common thing to do, so worth spending a few bytes of code to make it faster. And not really worth spending the bytes in all the other places that it's used.

@Jongy
Copy link
Contributor Author

Jongy commented Jul 15, 2018

Well, of course not to inline all invocations, but perhaps a inline __mp_obj_new_int and then mp_obj_new_int just calls it, so it is inlined there and in mp_binary_op.

@dpgeorge
Copy link
Member

perhaps a inline __mp_obj_new_int and then mp_obj_new_int just calls it, so it is inlined there and in mp_binary_op.

Feel free to try it out and submit a PR so it can be discussed in detail. But you may be interested in first reading #293.

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jan 21, 2021
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