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

Try to optimize a couple of things #1740

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

albinahlback
Copy link
Collaborator

This PR seems to be faster when ADX is available, but I had troubles getting consistent results. I don't know if these changes for fmpz_mul and fmpz_sqr are actually faster.

Perhaps there are things one could optimize, and if so, I would be happy to do so.

Perhaps X_mat_neg could have a X_mat_inplace_neg instead.

NOTE: I think the assertion worker will fail due to fmpz_mul and fmpz_sqr allowing aliasing under certain circumstances when ADX assembly is enabled.

@albinahlback
Copy link
Collaborator Author

Hmm, I think I have to split this into smaller PRs.

#include "mpn_extras.h"
#include "fmpz.h"

__GMP_DECLSPEC extern void * (*__gmp_allocate_func)(size_t);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we assume that GMP allocation functions are the same as FLINT's, so there shouldn't need to be some special magic in this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect there are already cases where we put a flint_malloc:ed pointer in an mpz. I could be wrong about this though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then why don't we simply define flint_malloc as __gmp_allocate_func?

@fredrik-johansson
Copy link
Collaborator

Hmm, I think I have to split this into smaller PRs.

Yes.

The inplace functions look nice though. Is foo_neg_inplace a better name than foo_inplace_neg?

@albinahlback
Copy link
Collaborator Author

The inplace functions look nice though. Is foo_neg_inplace a better name than foo_inplace_neg?

foo_method_inplace is nice because you can search for foo_method, and this will pop up. However, foo_inplace_method is somewhat more futureproof in case foo_inplace becomes it's own module. It is also nice to search for foo_inplace, and you see what inplace methods are available. I don't have a strong opinion here, I'll let you decide.

@fredrik-johansson
Copy link
Collaborator

foo_method_inplace is nice because you can search for foo_method, and this will pop up.

That is my thinking.

However, foo_inplace_method is somewhat more futureproof in case foo_inplace becomes it's own module.

It sounds extremely specialized to have whole modules just for inplace versions of functions.

It is also nice to search for foo_inplace, and you see what inplace methods are available.

I think you are more likely to search for versions of a specific operation than for a group of inplace operations.

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