Re: [PATCH v3 1/9] Add new exp and exp2 implementations

[Joseph Myers <joseph at codesourcery dot com>]

On Fri, 29 Jun 2018, Szabolcs Nagy wrote:

> +/* Handle inputs that may overflow or underflow when computing the result
> +   that is scale*(1+tmp), the exponent bits of scale might have overflown
> +   into the sign bit so that needs correction before sbits is used as a
> +   double, ki is only used to determine the sign of the exponent.  */
> +static inline double
> +specialcase (double_t tmp, uint64_t sbits, uint64_t ki)

The comments are missing important information needed for review of the 
patch and for people reading the code in future.

You refer to the result as "scale*(1+tmp)" (in GNU style, a comment 
referring to the value of a variable should put its name in uppercase).  
But what is "scale" here?  It's not one of the function arguments; it's an 
internal variable inside the function.  You need to say what the semantics 
of TMP, SBITS and KI are, and what the return value of the function means 
in terms of those function arguments.  You say "ki is only used to 
determine the sign of the exponent" but you don't say what the semantics 
of it are.  From the body of the function it appears to be that bit 31 is 
set for negative arguments, clear for positive, which is certainly not the 
default a reader would expect from a comment not saying so explicitly - 
without explicitly detailing the unusual way in which ki is used, the 
natural implication of the comment would be that it's bit 63 that is used.  
(And if the result is a function only of TMP and SBITS, KI needs to be 
explained in terms of e.g. overflow being possible in certain cases and 
underflow in others - if that's the intended semantics of KI.)

> +#if TOINT_INTRINSICS
> +  kd = roundtoint (z);
> +  ki = converttoint (z);

What are the semantics of these functions?  The definitions in 
sysdeps/aarch64/fpu/math_private.h have no comments explaining their 
semantics to facilitate review of code using them.  But really those 
definitions *shouldn't* be where the semantics are documented.  Rather, 
math_config.h (both the new one and the existing one in flt-32 - comments 
in one can reference comments in the other for details) should have 
comments explaining that, if an architecture defines TOINT_INTRINSICS to 
1, it must also define those functions with such-and-such semantics).  
That way you have the semantics defined in an architecture-independent 
place for the benefit of people reading the code and people possibly 
tuning it for other architectures in future.

There are sufficiently many different cases in the code (fma or not (maybe 
that one's more of an issue for some of the other patches), 
TOINT_INTRINSICS, EXP_USE_TOINT_NARROW, different values of EXP_POLY_ORDER 
and EXP_TABLE_BITS and EXP_POLY_WIDE) that I think clear information is 
needed about how those different cases have been tested and what purpose 
they all serve.  My starting expectation is that there should be one value 
of EXP_POLY_ORDER and one value of EXP_TABLE_BITS in glibc (and probably 
likewise for some other macros), without multiple alternative code choices 
in glibc (possibly with #error in appropriate cases to make clear they 
aren't supported in glibc), unless there is some reason different choices 
are appropriate for different architectures - there should not be lots of 
different cases in code and tables of data that are not actually used in 
any glibc configuration.

-- 
Joseph S. Myers
joseph@codesourcery.com