Re: [PATCH] Fix up strtod_l.c for -O0

["Carlos O'Donell" <carlos at redhat dot com>]

On 02/19/2013 06:19 AM, Jakub Jelinek wrote:
> Hi!
> 
> I know that glibc currently refuses to compile with -O0, but e.g.
> libquadmath that copies code from glibc doesn't enforce that.
> 
> As reported in http://gcc.gnu.org/PR56379 , when strtod_l.c is compiled with
> -O0, it misbehaves.  The problem is that __mpn_lshift_1's
> __builtin_constant_p (count) && count == BITS_PER_MP_LIMB
> isn't just an optimization as the comment says.  __mpn_lshift_1
> is normally called either with count equal to constant BITS_PER_MP_LIMB,
> and then it expects to do the inline loop, or with variable count
> that is assumed to be > 0 and < BITS_PER_MP_LIMB.
> __mpn_lshift_1 is always_inline, but __builtin_constant_p is just an
> optimization, so at -O0 it will evaluate to 0 when the argument isn't a
> constant already during the parsing (and only inlining in that case
> changes it into a constant).
> The reason why the else part doesn't handle count == BITS_PER_MP_LIMB
> properly is
> 1) mpn_lshift itself claims:
>    Argument constraints:
>    1. 0 < CNT < BITS_PER_MP_LIMB
>    2. If the result is to be written over the input, WP must be >= UP.
> 2) even ignoring that, by most of the assembly implementations it might
>    do the right thing, still ptr[0] will be usually unmodified original
>    value and ptr[0] |= limb >> (BITS_PER_MP_LIMB - count);
>    performs ptr[0] |= limb; rather than ptr[0] = limb; that would be
>    desirable for that count.
> 
> The reason for __builtin_constant_p is I think that Ulrich didn't want to
> bloat the code, while the compiler could sometimes with VRP figure out
> that for variable count it will be > 0 and < BITS_PER_MP_LIMB, it doesn't
> have to figure that out always and __mpn_lshift_1 code would then be bloated
> by handling both cases, even when we know that only one of them is ever
> needed at the particular call site.
> 
> So, to fix this, either we can change __mpn_lshift_1 into a macro, either
> partially using helper inlines as done in the following patch, or fully,
> with no inlines at all, or we could keep the always_inline __mpn_lshift_1,
> just use
>   if (
> #ifdef __OPTIMIZE__
>       __builtin_constant_p (count)
>       &&
> #endif
>          count == BITS_PER_MP_LIMB)
>     {
> ...
>     }
>   else
>     {
> ...
>     }
> so that for -O1+ builds we'd never have extra code in there, but for
> -O0 we would.
> 
> So, any preferences on this?  Or perhaps nothing at all, and we'll fix this
> in libquadmath independently.

No, we should fix this here.

I think a macro and two helper functions, as you have done below, is
a good solution and keeps the code from bit-rotting.

No inlines looks like it would be worse code.

Using conditionals on __OPTIMIZE__ will lead to bit-rot since we don't
currently compile glibc with anything but -O2.

> 2013-02-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* stdlib/strtod_l.c (__mpn_lshift_1_word, __mpn_lshift_1_var): New
> 	inline functions.
> 	(__mpn_lshift_1): Change into macro using the above functions.

So what does the generated code look like after the change?

Cheers,
Carlos.