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

Simplifications #434

Merged
merged 13 commits into from
Nov 5, 2019
Merged

Simplifications #434

merged 13 commits into from
Nov 5, 2019

Conversation

minad
Copy link
Member

@minad minad commented Oct 29, 2019

This PR contains two things:

  • Renaming some mp_(root|expt|log)_u32 functions back to suffix less version. Using uint32_t for those functions was a mistake, I have to admit that now. In particular using a suffix is not a good idea, since this is a slight obstacle now to change the types :( These functions should either take/return bitcounts (int) or mp_digits. For example mp_log is defined in terms of count_bits, so this is the natural type to use. mp_root calls mp_mul_d, so mp_digit would be a good fit. For now I am using simply int.

  • Many simplifications - I hope that those will be helpful when we decide to convert the bitcount type to size_t/or something else. Furthermore these simplifications are helpful if we want to change the digit representation, such that the full width is used.

@minad minad requested review from sjaeckel and czurnieden and removed request for sjaeckel October 29, 2019 17:03
@minad
Copy link
Member Author

minad commented Oct 29, 2019

@sjaeckel @czurnieden Since this is a huge PR touching a lot of functions, it might make sense to split it into smaller pieces, which are easier to consume, if that is desired.

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

had only a quick look for now, basically looks good

tommath.h Outdated Show resolved Hide resolved
tommath.h Outdated Show resolved Hide resolved
@minad
Copy link
Member Author

minad commented Oct 29, 2019

Generally, regarding this PR my proposal would be the following. I split it in three parts:

  1. The suffix PR (let's decide on what suffix to use again ;)
  2. One PR containing the more trivial refactorings about which I am very sure
  3. Refactorings of the more complicated functions, which need a more careful review.

@sjaeckel What do you think?

@sjaeckel
Copy link
Member

regarding this PR my proposal would be the following. I split it in three parts:

that was my first thought when I saw this huge PR

@minad
Copy link
Member Author

minad commented Oct 29, 2019

I splitted the more complicated parts of this PR in multiple commits regarding logical units of functions. These can also be extracted to separate PRs if desired. This PR is rebased on #435.

@minad minad force-pushed the simplifications branch 2 times, most recently from 94bb741 to f1779e7 Compare October 29, 2019 19:31
@minad
Copy link
Member Author

minad commented Oct 29, 2019

@czurnieden @sjaeckel If you want to open the renaming bottle, I would propose to rename mp_div_2d -> mp_rsh, mp_mul_2d -> mp_lsh and s_mp_log_pow2 -> s_mp_log_2expt. The _2d suffix is really weird. These renamings could even be backported to 1.3 with deprecations warnings if we want to ease the transition. The same applies to mp_(expt|log|root) without or with a new suffix. It is funny that we rename the functions again, but naming is really the hardest problem.

@czurnieden
Copy link
Contributor

mp_div_2d -> mp_rsh, mp_mul_2d -> mp_lsh

If you want to include the proposed changes (inclusion of the subfunctions like mul_2 and mp_lshd) it is quasi a new function which sells the name change better.

Ah, I forgot one: the data type for the shift amount is still int. Would it make sense to allow for negative input and and send that to the opposite function (like in mp_add and mp_sub) or would that behaviour too surprising?

s_mp_log_pow2 -> s_mp_log_2expt.

These are private functions, what you call them should not matter at all.

but naming is really the hardest problem.

But one of the many parental duties ;-)

@minad
Copy link
Member Author

minad commented Oct 29, 2019

Ah, I forgot one: the data type for the shift amount is still int. Would it make sense to allow for negative input and and send that to the opposite function (like in mp_add and mp_sub) or would that behaviour too surprising?

One could only provide only one shift function which decides based on the sign.
However this is not how things are usually, e.g., the >> and << operators. Furthermore there are different behaviors regarding sign extension for >>, where one operation does sign extension shift and one which extends with zeros.

I think it makes sense to keep it as is. Or rather return an error for negative shifts, this is what I did in this PR. mp_lshd and mp_rshd are special however. They ignore negative shifts since mp_rshd doesn't return an error code.

@minad
Copy link
Member Author

minad commented Oct 29, 2019

@czurnieden If you have time in the next days, I would highly appreciate if you review the things I did here, since this could easily lead to bugs. I did things step by step and used the test suite quite often, but things could have happened. Feel free to also push additional commits if something comes up.

@minad
Copy link
Member Author

minad commented Oct 29, 2019

@czurnieden One other thing regarding comba - since you asked about performance. Actually I don't expect there to be a huge difference between asm and the c version. In ltm we only have the generic comba and nothing unrolled. I think in tfm the performance difference comes mainly due to loop unrolling in combination with the custom assembly. Probably also due to vectorization.

@czurnieden
Copy link
Contributor

czurnieden commented Oct 30, 2019

I have no serious problems with the function mp_prime_strong_lucas_selfridge, s_mp_exptmod, s_mp_exptmod_fast. They look just very complex.

Took a short first glimpse, and can say with confidence: it needs a second glimpse and a much longer one, too.

BTW: I found that the cut-off values in mp_exptmod* for the number of windows a bit off from the theoretical values. Nothing serious, just wondering how they were computed in the first place because the differences seem to be too much for a simple rounding error.

1.246041023861365  :  1 
2.058660344689936  :  8             7  
3.001508362585475  :  41           36
4.002716871975187  :  162         140
5.000808585528096  :  530         450
6.000607510406698  :  1562       1303
7.000136942889815  :  4299       3529
8.000044561915664  :  11293     <- max. needed for current key-lengths in RSA plus one.
9.000030428030995  :  28666 
10.00001149850915  :  70879 
11.00000388290744  :  171647 
12.00000044772216  :  408691 
13.00000095004352  :  959458
14.00000022160508  :  2225683 
15.00000011060768  :  5110211
16.00000000335862  :  11628824
17.00000004155538  :  26255994
18.00000001397662  :  58871894
19.0000000058284  :  131190198
20.00000000346447  :  290726600
21.00000000150675  :  641052593
22.00000000078542  :  1407118193

Computed with the abomination below

tempfile=$(mktemp /tmp/maxima_out.XXXXXX) && maxima -q --batch-string='eqn5:-(a/x^2)+2^x * log(2) -1;for a:1 thru 1000000 step 1 do print(find_root(eqn5,x,1,20), " : ", a);' > $tempfile  && for i in {1..20};do grep -m1 ^$i $tempfile;done ; rm -v $tempfile

(Currently running up to 2^31 just because. Will add results when ready)

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.

So far, so good.

@minad
Copy link
Member Author

minad commented Nov 2, 2019

@sjaeckel this is good to merge!

@minad minad force-pushed the simplifications branch 3 times, most recently from cfc3761 to 70bc075 Compare November 3, 2019 18:26
@minad minad requested a review from sjaeckel November 3, 2019 18:28
Originally I made those as macros. However we have many
other small functions like mp_clamp, mp_exch which are also not implemented
as macros right now.

If we would use c99, I would implement them as private static inline
functions. And mp_exch would be a public static inline function.

But since we are bound to c89, we simply use normal functions.
To achieve optimal performance one should either use link time
optimization or amalgamation.
* these double checks are not necessary
* the compiler will move the early return outside of the called
  function, basically the functions is partially inlined
* however lto/amalgamation needed for the optimization
* this is the final commit of a series of simplifications,
  containing only the regenerated files and the explanation in the
  commit message

* This is in preparation of the size_t change/a potential representation change to use
  full width as in tfm, if a (partial?) merge with tfm is desired.
  These changes have their own merits however.

* Remove obfuscating tmpx digit pointers (fewer variables, it is more obvious what is
  being manipulated)

* Reduce scope of variables where possible

* Stricter error handling/checking (for example handling in karatsuba
  was broken)

* In some cases the result was written even in the case of an error
  (e.g. s_mp_is_divisible). This will hide bugs, since the user should
  check the return value (enforced by MP_WUR). Furthermore if the user
  accesses the non-initialized result, valgrind will complain for
  example. Global static analysis like coverity will also detect the issue.
  Therefore this improves the status quo.

* Introduce generic, private MP_EXCH macro which can be used to swap values.

* Introduce s_mp_copy_digs/s_mp_zero_digs/s_mp_zero_buf

* Some control flow simplifications, e.g, loops instead of goto

* Renamings of variables/labels for consistency

* Renamings of mul/sqr functions for more consistency, e.g., comba
  instead of fast suffix

* I didn't read through some very complex functions.
  They are so complex, I am too afraid and lazy to touch them.
  Maybe someone resposible wants to simplify them if possible. Hint... Hint...
  - mp_prime_strong_lucas_selfridge.c
  - s_mp_exptmod.c
  - s_mp_exptmod_fast.c
@sjaeckel sjaeckel merged commit 3035e22 into develop Nov 5, 2019
@sjaeckel sjaeckel deleted the simplifications branch November 5, 2019 16:55
@sjaeckel
Copy link
Member

sjaeckel commented Nov 5, 2019

sorry for the delay, I was sick for the last week :-\

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.

3 participants