Skip to content

Conversation

@czurnieden
Copy link
Contributor

Removed the test that checks if the number of M-R rounds is smaller MP_PRIME_MAX
Should repair the test failure in #269

@minad minad mentioned this pull request May 19, 2019
@minad
Copy link
Member

minad commented May 19, 2019

Thanks for clarifying what you meant. I wondered about this t > MP_PRIME_SIZE condition.

A related question - would it make sense to remove all the code referencing on MP_PRIME_SIZE/ltm_prime_tab everywhere?

  • mp_prime_is_prime searches the table, is probably not worth it? --> remove the search
  • mp_prime_next_prime searches the table --> remove the search
  • mp_prime_is_divisible -->keep it as deprecated (als deprecate ltm_prime_tab, see API discussions/overhaul #243)

You argued before that this ltm_prime_tab is used for MP_8BIT compatibility/optimization. It seems that the is_prime function also bails out for MP_8BIT after searching the table. This special casing would then go to or what is the advantage of having that?

@czurnieden
Copy link
Contributor Author

Would it make sense to remove all the code referencing on MP_PRIME_SIZE/ltm_prime_tab everywhere?

Once we have a sieve? Of course.

But be aware of the uncomfortable fact that that would change the API of mp_prime_is_divisible, and mp_prime_is_prime.c because they need to receive the sieve from the caller. It would also make single use of those functions more expensive.

This special [MP_8BIT] casing would then go to or what is the advantage of having that?

For on 7-bit large limb testing the first 31 primes is exhaustive, these are all primes that are in a single 7-bit limb, all primes <= 127.

@minad
Copy link
Member

minad commented May 19, 2019

But be aware of the uncomfortable fact that that would change the API of mp_prime_is_divisible, and mp_prime_is_prime.c because they need to receive the sieve from the caller. It would also make single use of those functions more expensive.

Hmm, okay. I would probably deprecate mp_prime_is_divisible/ltm_prime_tab and make them internal as s_mp_prime_is_divisible/s_mp_prime_tab. Then mp_prime_is_prime could still use it for simplicity. Then we add your new sieve code. Could we then restructure the internals such that maybe some kind of preallocated sieve is used instead of s_prime_is_divisible?

@czurnieden
Copy link
Contributor Author

Could we then restructure the internals such that maybe some kind of preallocated sieve is used instead of s_prime_is_divisible?

Getting the primes for s_prime_is_divisible from a different source won't remove the need for the function of s_prime_is_divisible itself: the trial division. A sieve as the source for the primes allows for more primes to be tested which is useful for factorization where the next level up, e.g.: Pollard-Rho, is more expensive than say a trial division with the first 1,000 primes.

Re. global sieve: Only the use of the base segment could be made thread-safe without much work (read-only), building and removing segments would need some safeguards and would go outside of c89.

BTW: it would be simpler if we have the sieve in develop already and I could just offer a PR to show what is possible. It makes a discussion easier if you can quote line&column of the code you are talking about. I don't want to use the branch bn_sieve itself for it.
And if you (plural, it's more than the two of us here ;-) ) do not want a sieve in the first place this whole discussion would be mood.

I don't know for sure if I have all my stuff uploaded to my github repository and I'm still on my old laptop until tonight, so no rebasing of bn_sieve yet.

Ah, Travis tests succeeded, you can repair #269 now if you want (he said boldly ;-) )

@minad
Copy link
Member

minad commented May 21, 2019

@czurnieden Could you push this on top of #269 or should @sjaeckel do that?

@sjaeckel sjaeckel merged commit d46cb16 into libtom:develop May 21, 2019
@czurnieden
Copy link
Contributor Author

Ah, already done. Thanks!

BTW: 7:08AM? Do you know what I get to hear if somebody sees me at such a time? "Ow, c'mon, go to bed, you ain't no 20 anymore!" ;-)

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.

3 participants