gmpxx: bug in declaration of __gmp_set_expr

[Torbjorn Granlund]

Marc Glisse <marc.glisse at normalesup.org> writes:

  etc with all possible combinations of mpz/mpq/mpf and
  mp*_class/__gmp_expr.
  
  First, one can notice in the rest of the code that __gmp_set_expr is
  mostly called on compound expressions, and can only be called on
  mp*_class if it is not the same as the first argument (mpz_t,mpz_class
  is never called, but mpz_t,mpq_class can be). This means three of
  these functions are useless, but they don't hurt and might become
  useful again soon. Now if we actually call
  __gmp_set_expr(mpz_t,mpq_class), we notice that it does not call the
  version of the function that we would expect, but the generic
  __gmp_set_expr(mpz_t,__gmp_expr(mpq_t,T)). The reason appears to be
  the empty "template<>" at the beginning. It looks like a confusion
  between functions (which get overloads) and classes (which get partial
  specializations). Removing the "template<>" seems to solve the
  issue. But then it is surprising the compiler didn't print a warning
  if writing it was wrong. Also, the early declarations are apparently
  useless (but if we want them we should probably declare all
  overloads).
  
  It would be nice if someone familiar with C++ could comment on this...
  
Indeed.  :-)  
  
  By the way I am attaching a version of gmpxx.h where I played with
  various bits:
  - small temporaries on stack (I skipped hypot)

Like the stuff I did recently?

  - no need to write the code 4 times for all comparison operators
  - do some unary operations in-place (except for sqrt for which it
  looks slower)
  I am not too happy with the commute thing.
  Don't take any of it as-is, it is just there as a list of ideas, even
  though I made sure it still passed the testsuite.

The tests for gmpxx.h in the GMP testsuite is far inferior to the rest
of the testsuite, unfortunately.

  I am also attaching
  a patch to mpz/aors to make reallocation less likely, but I am not
  sure it really improves anything with a usual malloc implementation
  where realloc(size+4) is usually cheap.

I am not sure this is a good idea, since it might lead to two
reallocations.  *If* it is a good idea, the same trick applies to
dozens of other places in GMP.

It is an intensional design to end up with an extra limb, to simplify
bookkeeping.  It might be a bad design decision for applications with
a large number of small mpz_t variables, so we might want to evaluate
it.  But then we need to look at the cost of it in many functions, not
just mpz/aors.
  
  In gmpxx, it sometimes happens that mpq_set(r,s) is called with r==s
  in addition and substraction (for instance for q+=3), but I noticed
  that g++ -O1 is able to detect it with __builtin_constant_p. We could
  replace mpq_set by if(r!=s)mpqset, but if we decide there is too much
  overhead this way, we can still do it at compile time only with gcc.

How do you use __builtin_constant_p for that?

I don't like if(r!=s)mpqset much.

  There are actually quite a few things (even in the C version) that
  could benefit from the use of gcc's __builtin_constant_p. mul_ui could
  check whether its argument is a power of 2 and call mul_2exp
  instead.

That would require to make it a macro in gmp-h, of course, and rename
the function to something else.  Care need to be taken to put a
function named mpz_mul_ui in the (shared) library, otherwise we would
break binary compatibility.

But if that is taken care of, it could be a nice optimization.

  Oh and gmpxx seems to be missing a pow function.
  
That's fixable.

-- 
Torbjörn