Re: [PATCHES] Numeric improvements

[Mark H Weaver]

Hi Ludovic,

address@hidden (Ludovic Courtès) writes:

> Which of these patches are needed for mini-gmp integration?  It would
> probably be easier to discuss things separately, by small chunks.

Mini-gmp integration depends directly on patches 5 and 7, and those
depend on patches 2, 3, and 4.

> Overall I think I’m mostly incompetent on the numeric stuff, so I’d
> mostly comment on form.
>
> At first sight, it seems that there are few tests added, in particular
> for the number printer.  I think tests must be added along with the
> patches that claim to fix something.

Agreed.  I'll work on that.

>> From cd9784ed33d78e6647a752123bf7be91d65b5c96 Mon Sep 17 00:00:00 2001
>> From: Mark H Weaver <address@hidden>
>> Date: Sun, 3 Mar 2013 04:34:17 -0500
>> Subject: [PATCH 01/10] Improve code in scm_gcd for inum/inum case
>>
>> * libguile/numbers.c (scm_gcd): Improve implementation of inum/inum case
>>   to be more clear and efficient.
>
> This one looks OK, and should be covered by the tests, according to
> <http://hydra.nixos.org/build/4268423/download/2/coverage/libguile/numbers.c.gcov.html>.
>
> Did you measure the performance difference?

Yes.  On my x86_64 machine it improves gcd performance on fixnums by
about 4.25%.  I went ahead and pushed this one.

> Can you fold the explanations as comments?
[...]
> Please use imperative-tense sentences above functions to describe what
> they do, without repeating the function name; also make sure to
> introduce the arguments in the description, written in capital letters
> (info "(standards) Comments").
>
> Some helper functions introduced by the patches lack a comment.

Will do.

>> address@hidden {Scheme Procedure} round-ash n count
>> address@hidden {C Function} scm_round_ash (n, count)
>> +Return @math{round(@var{n} * address@hidden)}, but computed
>> +more efficiently.  @var{n} and @var{count} must be exact
>> +integers.
>> +
>> +With @var{n} viewed as an infinite-precision twos-complement
>> +integer, @code{round-ash} means a left shift introducing zero
>> +bits when @var{count} is positive, or a right shift rounding
>> +to the nearest integer (with ties going to the nearest even
>> +integer) when @var{count} is negative.  This is a rounded
>> +``arithmetic'' shift.
>> +
>> address@hidden
>> +(number->string (round-ash #b1 3) 2)     @result{} \"1000\"
>> +(number->string (round-ash #b1010 -1) 2) @result{} \"101\"
>> +(number->string (round-ash #b1010 -2) 2) @result{} \"10\"
>> +(number->string (round-ash #b1011 -2) 2) @result{} \"11\"
>> +(number->string (round-ash #b1101 -2) 2) @result{} \"11\"
>> +(number->string (round-ash #b1110 -2) 2) @result{} \"100\"
>> address@hidden lisp
>> address@hidden deffn
>
> What was the rationale for ‘round-ash’?

Well, we need it internally to efficiently convert large integers to
floating-point with proper rounding (see the call to
'round_right_shift_exact_integer' in patch 5).  Given that, it seemed
reasonable to export it to the user, since it's not possible to do that
job efficiently with our current primitives.  However, I don't feel
strongly about it.

> It seems to address to do two things differently from ‘ash’: it’s more
> efficient, and it rounds when right-shifting.  The “more efficient” bit
> is an implementation detail that should also benefit to ‘ash’ (as a user
> I don’t want to have to choose between efficient and non-rounding.)

No, 'round-ash' is not more efficient than 'ash'.  It's more efficient
than the equivalent (round (* n (expt 2 count))).

Thanks for looking through the patches.  I'll work on improving them.

     Mark