Re: [PATCH 0/3] sparc M7 optimized memcpy/memset

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 02/11/2017 13:17, Patrick McGehearty wrote:
> On 10/26/2017 2:39 PM, Adhemerval Zanella wrote:
>>
>> On 19/10/2017 14:46, Patrick McGehearty wrote:
>>
>>>> Adhemerval asked that the ifunc selector code use C instead of assembly.
>>>> He has prepared changes to replace all assembly uses of ifunc with C.
>>> The existing ifunc code in that directory uses assembly.
>>> Adhemerval's changes were not available when I submitted the patch.
>>> As far as I understand, they are still not available in the main branch.
>>> Making those changes increases risk for my memcpy/memset optimizations.
>>> I consider such work, while worthy in its own right, to be out of
>>> scope for my patch set.
>>>
>>> Respectively,
>>> - Patrick McGehearty
>> I just sent a patchset to refactor all remaining IFUNC resolver still
>> in assembly to C, including all the missing sparc ones.
>>
>> Also, I adjusted your patches to my refactor in personal branch [2] and
>> I also split the patch in memcpy/memmove and memset/bzero. It simplifies
>> a lot new ifunc inclusions, for instance the memcpy part is just:
>>
>> ---
>> diff --git a/sysdeps/sparc/sparc64/multiarch/ifunc-memcpy.h b/sysdeps/sparc/sparc64/multiarch/ifunc-memcpy.h
>> index 46f3795..dbdad2d 100644 (file)
>> --- a/sysdeps/sparc/sparc64/multiarch/ifunc-memcpy.h
>> +++ b/sysdeps/sparc/sparc64/multiarch/ifunc-memcpy.h
>> @@ -19,6 +19,7 @@
>>     #include <ifunc-init.h>
>>   +extern __typeof (REDIRECT_NAME) OPTIMIZE (niagara7) attribute_hidden;
>>   extern __typeof (REDIRECT_NAME) OPTIMIZE (niagara4) attribute_hidden;
>>   extern __typeof (REDIRECT_NAME) OPTIMIZE (niagara2) attribute_hidden;
>>   extern __typeof (REDIRECT_NAME) OPTIMIZE (niagara1) attribute_hidden;
>> @@ -28,6 +29,8 @@ extern __typeof (REDIRECT_NAME) OPTIMIZE (ultra1) attribute_hidden;
>>   static inline void *
>>   IFUNC_SELECTOR (int hwcap)
>>   {
>> +  if (hwcap & HWCAP_SPARC_ADP)
>> +    return OPTIMIZE (niagara7);
>>     if (hwcap & HWCAP_SPARC_CRYPTO)
>>       return OPTIMIZE (niagara4);
>>     if (hwcap & HWCAP_SPARC_N2)
>> ---
>>
>> So if you could help with any review I will be thankful. I would expect
>> the memcpy/memmove and memset/bzero refactor to be straightforward.
>>
>> [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/ifunc-c-sparc-m7
> I've reviewed Adhemerval's changes including building
> and running on my sparc s7 system. They look good to me.
> Only issue I see is a naming difference in sysdeps/sparc/sparc64/multiarch
> between:
> ifunc-memmove.c
> memcpy.c
> mempcpy.c
> memset.c
> 
> Perhaps ifunc-memmove.c should be named memmove.c for consistency
> with memset.c, mempcpy.c and memcpy.c?  Or all named ifunc-mem*.c?

Indeed this naming is a mistake and I will correct it before send it upstream
(sysdeps/sparc/sparc64/multiarch/ifunc-memmove.c should be indded
sysdeps/sparc/sparc64/multiarch/memmove.c).
It also shown a missing #endif at the end the file, thanks.