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

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

On 02/19/2013 08:18 AM, Jakub Jelinek wrote:
> On Tue, Feb 19, 2013 at 08:02:06AM -0500, Carlos O'Donell wrote:
>> 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.
> 
> Why?  macro vs. inline function should be generally the same code quality if
> the inline function is inlined, or perhaps macro could be slightly better if
> early optimizations before inlining would make a difference.
> But as it is always_inline, it generally should be inlined during early
> inlining, so the difference should be minimal.

The macro makes the code available earlier for optimization, that's
all, and I agree that the difference is likely minimal. Thus all things
considered I prefer a macro.

Is there any reason not to use a macro?

>> 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?
> 
> Haven't tried it on glibc, but on libquadmath the difference the patch does
> at -O2 is just two spots where two instructions were swapped:
> -    1b51:      45 89 cf                mov    %r9d,%r15d
> -    1b54:      4c 89 64 24 38          mov    %r12,0x38(%rsp)
> +    1b51:      4c 89 64 24 38          mov    %r12,0x38(%rsp)
> +    1b56:      45 89 cf                mov    %r9d,%r15d
> ...
> -    1bf0:      48 8b 5c 24 30          mov    0x30(%rsp),%rbx
> -    1bf5:      4c 8b 64 24 38          mov    0x38(%rsp),%r12
> +    1bf0:      4c 8b 64 24 38          mov    0x38(%rsp),%r12
> +    1bf5:      48 8b 5c 24 30          mov    0x30(%rsp),%rbx

So just a little bit of code motion and nothing else, this looks good
to me then.

Could you confirm that you see the same thing with a glibc build and
then checkin your changes on Wednesday (gives another day of review)?

Cheers,
Carlos.