Skip to content

Conversation

@minad
Copy link
Member

@minad minad commented Nov 6, 2019

Do we want that or do we want the user to explicitly call mp_sqr if the sqr function should be used?
GMP does this optimization I think, but they also don't expose a sqr function in the API.

@minad
Copy link
Member Author

minad commented Nov 6, 2019

@nijtmans For example in Tcl, does it use mp_sqr there or always calls mp_mul? Then it might make sense to push this optimization to ltm.

Furthermore we could think about removing mp_sqr from the public api alltogether and replacing it by a macro mp_mul(a, a, b) given we merge this PR.

@minad
Copy link
Member Author

minad commented Nov 6, 2019

The advantage of making sqr purely an internal optimization of sqr is certainly that if the lib is downstripped to the bare minimum, the sqr functions could be removed completely. And mp_sqr could still be used via the macro.

@MasterDuke17
Copy link
Collaborator

MasterDuke17 commented Nov 6, 2019 via email

Copy link
Contributor

@czurnieden czurnieden left a comment

Choose a reason for hiding this comment

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

Yes, but deprecate it formally, please, if you want to privatize mp_sqr.

@minad
Copy link
Member Author

minad commented Nov 6, 2019

Yes, but deprecate it formally, please, if you want to privatize mp_sqr.

We will do deprecations in 1.x when we remove the functions in develop. I will prepare a backport for that.

@minad
Copy link
Member Author

minad commented Nov 6, 2019

I also made mp_div_3 internal since it is only an optimization. I added more optimizations to mp_div_d and mp_mul_d.

@minad minad added the finished label Nov 6, 2019
@minad minad changed the title optimize mp_mul(a, a, b) by using mp_sqr make mp_sqr and mp_div_3 internal and add optimizations Nov 6, 2019
@minad minad requested a review from czurnieden November 6, 2019 16:02
Copy link
Contributor

@czurnieden czurnieden left a comment

Choose a reason for hiding this comment

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

Yepp, looks nice.

@minad minad mentioned this pull request Nov 7, 2019
Copy link
Collaborator

@nijtmans nijtmans left a comment

Choose a reason for hiding this comment

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

Tcl only uses mp_sqr() in 2 lines of code, so that's 2 minutes change-time. The less API's the better, so I agree with this change

@minad
Copy link
Member Author

minad commented Nov 7, 2019

mp_sqr will stay as macro btw. The point is really API surface reduction.

@minad
Copy link
Member Author

minad commented Nov 7, 2019

@sjaeckel this is ready from my side

@minad
Copy link
Member Author

minad commented Nov 9, 2019

@sjaeckel Rebased

@sjaeckel sjaeckel merged commit 0bc5c32 into develop Nov 10, 2019
@sjaeckel sjaeckel deleted the sqr-opt branch November 10, 2019 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants