Re: [PATCH v3] PowerPC: stpcpy optimization for PPC64/POWER7

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

On 24-10-2013 13:46, Will Schmidt wrote:
>
> A few comments scattered about below on cosmetic issues.   Nothing of
> real concern, this looks good to me.  
>
> Thanks, 
> -Will
Thanks for the review/comments Will.

> +#ifndef USE_AS_STPCPY
> +/* Save the dst pointer to use as return value.  */
> +	mr	rRTN, r3
> +#endif
> Just to get rid of a #ifndef, I'd be tempted to change the above to just
> 	mr	rRTN, r3
> and live with the "mr r3, r3" for the USE_AS_STPCPY case.
>
> I Don't feel strongly about it, but wanted to have more feedback than
> 'looks good' :-)
Yeah, although we can live, it will be more icache usage and another instruction to
dispatch (although in the end it might turn in nop).

>
>> +
>> +#ifndef USE_AS_STPCPY
>> +libc_hidden_builtin_def (strcpy)
>> +#endif
> I'm not sure if there is style precedence, can or should the #ifndef
> above be dropped, so that reads 
> 	libc_hidden_builtin_def (FUNC_NAME)
> And then remove the statement from the stpcpy.S that includes this file?
>
> I suppose that would be a problem if we needed to mix
> libc_hidden_builtin_def and libc_builtin_def incantations...  
>
>
> Looks Ok. 
I in fact try to mimic what x86_64 does in a lot of assembly code (check sysdeps/x86_64/memcpy.S
for instance).
>
> Similar comment as earlier, should the strcpy reference here become
> FUNC_NAME, and possibly also remove the #ifndef wrapper.
>
I tried to keep the libc_hidden_xxx/strong_alias/weak_alias in the same modules as they are defined.

> Either way, looks OK. 
> Thanks, 
> -Will

If you don't have any more objection, I'll commit it.