Hi Ludovic, l...@gnu.org (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 <m...@netris.org> >> 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. >> +@deffn {Scheme Procedure} round-ash n count >> +@deffnx {C Function} scm_round_ash (n, count) >> +Return @math{round(@var{n} * 2^@var{count})}, 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. >> + >> +@lisp >> +(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\" >> +@end lisp >> +@end 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
