Re: [PATCH] powerpc: P9 vector load instruction change in memcpy and memmove

["Tulio Magno Quites Machado Filho" <tuliom at linux dot vnet dot ibm dot com>]

Florian Weimer <fweimer@redhat.com> writes:

> On 10/19/2017 05:25 PM, Tulio Magno Quites Machado Filho wrote:
>> diff --git a/sysdeps/powerpc/powerpc64/multiarch/bcopy.c b/sysdeps/powerpc/powerpc64/multiarch/bcopy.c
>> index 05d46e2..4a4ee6e 100644
>> --- a/sysdeps/powerpc/powerpc64/multiarch/bcopy.c
>> +++ b/sysdeps/powerpc/powerpc64/multiarch/bcopy.c
>> @@ -22,8 +22,12 @@
>>   extern __typeof (bcopy) __bcopy_ppc attribute_hidden;
>>   /* __bcopy_power7 symbol is implemented at memmove-power7.S  */
>>   extern __typeof (bcopy) __bcopy_power7 attribute_hidden;
>> +/* __bcopy_power9 symbol is implemented at memmove-power9.S.  */
>> +extern __typeof (bcopy) __bcopy_power9 attribute_hidden;
>>   
>>   libc_ifunc (bcopy,
>> -            (hwcap & PPC_FEATURE_HAS_VSX)
>> +	    (hwcap2 & PPC_FEATURE2_ARCH_3_00)
>> +	    ? __bcopy_power9
>> +	    : (hwcap & PPC_FEATURE_HAS_VSX)
>>               ? __bcopy_power7
>>               : __bcopy_ppc);
>> diff --git a/sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c b/sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c
>> index 6a88536..9040bbc 100644
>> --- a/sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c
>> +++ b/sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c
>> @@ -51,6 +51,8 @@ __libc_ifunc_impl_list (const char *name, struct libc_ifunc_impl *array,
>>   #ifdef SHARED
>>     /* Support sysdeps/powerpc/powerpc64/multiarch/memcpy.c.  */
>>     IFUNC_IMPL (i, name, memcpy,
>> +	      IFUNC_IMPL_ADD (array, i, memcpy, hwcap2 & PPC_FEATURE2_ARCH_3_00,
>> +			      __memcpy_power9)
>>   	      IFUNC_IMPL_ADD (array, i, memcpy, hwcap & PPC_FEATURE_HAS_VSX,
>>   			      __memcpy_power7)
>>   	      IFUNC_IMPL_ADD (array, i, memcpy, hwcap & PPC_FEATURE_ARCH_2_06,
>> @@ -65,6 +67,8 @@ __libc_ifunc_impl_list (const char *name, struct libc_ifunc_impl *array,
>>   
>>     /* Support sysdeps/powerpc/powerpc64/multiarch/memmove.c.  */
>>     IFUNC_IMPL (i, name, memmove,
>> +	      IFUNC_IMPL_ADD (array, i, memmove, hwcap2 & PPC_FEATURE2_ARCH_3_00,
>> +			      __memmove_power9)
>>   	      IFUNC_IMPL_ADD (array, i, memmove, hwcap & PPC_FEATURE_HAS_VSX,
>>   			      __memmove_power7)
>>   	      IFUNC_IMPL_ADD (array, i, memmove, 1, __memmove_ppc))
>> @@ -168,6 +172,8 @@ __libc_ifunc_impl_list (const char *name, struct libc_ifunc_impl *array,
>>   
>>     /* Support sysdeps/powerpc/powerpc64/multiarch/bcopy.c.  */
>>     IFUNC_IMPL (i, name, bcopy,
>> +	      IFUNC_IMPL_ADD (array, i, bcopy, hwcap2 & PPC_FEATURE2_ARCH_3_00,
>> +			      __bcopy_power9)
>>   	      IFUNC_IMPL_ADD (array, i, bcopy, hwcap & PPC_FEATURE_HAS_VSX,
>>   			      __bcopy_power7)
>>   	      IFUNC_IMPL_ADD (array, i, bcopy, 1, __bcopy_ppc))
>
> I'm concerned that this needs *another* change to recognize post-DD2.1 
> POWER 9 hardware which has the fix (or perhaps OpenPOWER implementations 
> which never had the bug).
>
> Is there any other way you could select the workaround that is more 
> specific?

In userspace, fast and simple to use in an IFUNC resolver, no.
AFAIK, it would require to parse /proc/cpuinfo.

> Or is the performance hit from avoiding the affected vector 
> instructions not so severe that it would be an immediate concern for 
> future silicon?

The instructions Raji chose to use here do not impact the performance on
current silicon and is not expected to impact future silicon as well.

-- 
Tulio Magno