Extension of mp_log to bigint bases

[czurnieden]

(Replaces #495 because I messed that one up, sorry!)

My old method suffers from OVF errors that are not salvageable, at least not easily. Replaced it with a very simple one where the overflow can be caught (hard to explain, see code). It is also possible now to use a bigint as the base. I hope I didn't mess up as much as I do regularly.

The proof is in the pudding.

[czurnieden]

The proof isn't in the pudding anymore, I removed my…uhm… concatenation of notes because it is three years now and if I haven't done it by now, I never will.

It is a bit more complicated now but it is reasonable fast and gives valid results for the full range, the old binary search method overflowed quickly and was rather slow (a lot of expensive exponentiations).

[sjaeckel]

I've just compared the two mp_log() implementations side-by-side and they're basically the same, besides some small differences ... I think it would be better suited if they were refactored into

tommath_private.h:
---
#if ((UINT_MAX == UINT32_MAX) && (MP_WORD_SIZE > 4)) \
      || ((UINT_MAX == UINT16_MAX) && (MP_WORD_SIZE > 2))
#define S_MP_FP_LOG_D_POSSIBLE_C
#endif
---

s_mp_fp_log.c
---
static mp_err s_mp_fp_log_fraction(const mp_int *a, int p, mp_int *c) {...}
mp_err s_mp_fp_log(const mp_int *a, mp_int *c) {...}
---

s_mp_fp_log_d.c
---
static mp_word s_mp_flog2_mp_word(mp_word value) {...}
static mp_err s_mp_fp_log_fraction(mp_word x, int p, mp_word *c) {...}
mp_err s_mp_fp_log_d(const mp_int *a, mp_word *c) {...}
---

mp_log.c:
---
static mp_err s_approx_log(...) {
   ...
   s_mp_fp_log(...)
   ...
}
static mp_err s_approx_log_d(...) {
   ...
   s_mp_fp_log_d(...)
   ...
}

mp_err mp_log(const mp_int *a, const mp_int *b, int *lb)
{
   /* preamble ... error checks, mp_count_bits(), etc */
   if (MP_HAS(S_MP_LOG_D_POSSIBLE))
      s_approx_log_d(...);
   else
      s_approx_log(...);
   /* Check result. Result is wrong by 2(two) at most. */
   /* ... and so on */
---

Like that we always compile all versions of the API and could in theory even write tests against both s_mp_fp_log() and s_mp_fp_log_d().

What do you think?

[czurnieden]

I think it would be better suited if they were refactored into […]

Makes sense, will refactor as recommended.
Thanks for your time!
(May need a day or two more, am on my old Thinkpad now which seems to have some (electrical) trouble with the touchpad which is a bit annoying)

BTW: where's the rest of the gang? Always there when it is all new and interesting with chances to shine but once it's time to shove the broom through the shop they are all gone and the dirty work of lubing the machines is left to the old warhorses like us, eh? ;-)

[czurnieden]

Did the refactoring as I understood it.

Tests for the fixed point log_2 functions need a couple of testvalues (don't want to write a fixed point exponential function) that have to be contructed to cover the full range (as long as reasonable, a number with MP_MAX_DIGIT_COUNT can get quite large depending on MP_xxBIT)

[sjaeckel]

Did the refactoring as I understood it.

Thanks!

I've refactored a bit further, can you please have a look?

[czurnieden]

(Took the liberty to change the label from "finished" to "in progress" )

Ah, it was the all the same, wasn't sure when I moved it all around (I'm not a morning person, sorry ;-) ), good.

I'm still thinking about inverting the logic and giving that macro a different name.

I'm not the greatest fan of negative conditions, but…

If we would change that, we could add a CI job that tests the second code path and we wouldn't need separate tests for the private APIs.

…that's a very good reason.

But I don't like S_MP_FP_LOG_D_NOT_POSSIBLE_C grinning
What do you think?

I think that programmers should abstain from naming things ;-)

The mp_log functions rely on the size of mp_word being larger than INT_MAX. Mmh…

• MP_WORD_SMALLER_THAN_INT_MAX? Nah, awkward.
• INT_MAX_LT_MP_WORD? But we want the negative
• MP_WORD_LE_INT_MAX? I don't know.

Conclusion: I think that programmers should abstain from naming things.

Why did "-m64" choke just because that define was moved? scratches head

[czurnieden]

Why did "-m64" choke just because that define was moved? scratches head

Because I shouldn't trust the Github GUI. Why do I always make the same error? There, in the logs, it clearly states: "test.c:1500" where we test the small edgecases and from that is was obvious that mp_init(&bn) had to move down. But the GUI makes it look as if the movement of the define was the culprit!

[czurnieden]

Argh, overlooked, *grr*
Thanks!

[sjaeckel]

The mp_log functions rely on the size of mp_word being larger than INT_MAX. Mmh…

• MP_WORD_SMALLER_THAN_INT_MAX? Nah, awkward.
• INT_MAX_LT_MP_WORD? But we want the negative
• MP_WORD_LE_INT_MAX? I don't know.

MP_WORD_TOO_SMALL maybe? or MP_WORD_TOO_SMALL_FOR_LOG_D?

[czurnieden]

MP_WORD_TOO_SMALL maybe?

Yepp, that's nice, let's take it.

[czurnieden]

MP_WORD_TOO_SMALL_FOR_LOG_D

Too much wiggle room for innuendos, sorry.

On the other side: "Too small for what?" if we use MP_WORD_TOO_SMALL alone, so…ok.

[sjaeckel]

Too much wiggle room for innuendos, sorry.

On the other side: "Too small for what?" if we use MP_WORD_TOO_SMALL alone, so…ok.

I've just tried it out locally and defining S_MP_WORD_TOO_SMALL at compile time doesn't trigger the other code path ... I'm a bit surprised, but I'm also hungry, so that's for tomorrow or another time to figure out :)

[czurnieden]

I've just tried it out locally and defining S_MP_WORD_TOO_SMALL at compile time doesn't trigger the other code path

Yes, the MP_HAS() construct is quite complicated, took me a while.

The construct resolves to sizeof("""""") == 1u if the preprocessor definition is inside the code and to sizeof("""1""") == 1u if given at the commandline. The former is 1 because there is only the \0 and the latter is 2 because the "1" character counts as one plus the \0 character are 2. (a bit oversimplified)

Try giving it an empty string explicitely LTM_CFLAGS=' -DS_MP_WORD_TOO_SMALL_C="" ', seems to work.

But I don't know if that is the correct workaround.

[czurnieden]

Put it up to the community but don't hold your breath, I rarely have a lot of success with that. And, yes, I am aware of the different code licenses.

[czurnieden]

Nope, didn't work. *grr*

[czurnieden]

Yepp, works now. The S_MP_WORD_TOO_SMALL_C switch I mean.
Did only one with m64 plus valgrind and the two small limbsizes, didn't want to make an extra loop and include all.
Enough? Too much?

[sjaeckel]

Yepp, works now. The S_MP_WORD_TOO_SMALL_C switch I mean.
Did only one with m64 plus valgrind and the two small limbsizes, didn't want to make an extra loop and include all.
Enough? Too much?

More than I would've done, less than is possible -> perfect :)

I took the liberty to squash those last few commits of you together into one.