Re: [PATCH] AArch64: Improve strlen_asimd performance (bug 25824)

[Carlos O'Donell <carlos@redhat.com>]

On 7/16/20 12:52 PM, Szabolcs Nagy wrote:
> The 07/16/2020 11:31, Carlos O'Donell wrote:
>> On 7/16/20 9:00 AM, Wilco Dijkstra wrote:
>>> Optimize strlen using a mix of scalar and SIMD code. On modern micro
>>> architectures large strings are 2.6 times faster than existing strlen_asimd
>>> and 35% faster than the new MTE version of strlen.
>>>
>>> On a random strlen benchmark using small sizes the speedup is 7% vs
>>> strlen_asimd and 40% vs the MTE strlen.  This fixes the main strlen
>>> regressions on Cortex-A53 and other cores with a simple Neon unit
>>> (see https://sourceware.org/pipermail/libc-alpha/2020-June/114641.html )
>>>
>>> Rename __strlen_generic to __strlen_mte, and select strlen_asimd when
>>> MTE is not enabled (this is waiting on support for a HWCAP_MTE bit
>>> which can hopefully be added soon).
>>>
>>> This fixes big-endian bug 25824. Passes GLIBC regression tests.
>>>
>>> I'd like this for 2.32 to fix the bug and avoid any regressions.
>>
>> The copyright/description changes don't look correct.
>>
>> Overall this is OK for 2.32, but Szabolcs should review this also.
> ...
>>> diff --git a/sysdeps/aarch64/multiarch/strlen.c b/sysdeps/aarch64/multiarch/strlen.c
>>> index 99f2cf2cde54fd1158383d097ba51edc1377f55b..7c0352dd878086708ac785807bc4d210b85e528f 100644
>>> --- a/sysdeps/aarch64/multiarch/strlen.c
>>> +++ b/sysdeps/aarch64/multiarch/strlen.c
>>> @@ -26,17 +26,15 @@
>>>  # include <string.h>
>>>  # include <init-arch.h>
>>>  
>>> -#define USE_ASIMD_STRLEN() IS_FALKOR (midr)
>>> +/* This should check HWCAP_MTE when it is available.  */
>>> +#define MTE_ENABLED() (false)
>>
>> OK.
> 
> i'd like glibc 2.32 to support heap tagging via malloc
> interposition (on future linux + future hw).
> 
> MTE_ENABLED==false in the ifunc dispatch prevents that.
> (we select non-mte-safe strlen)
> 
> is adding the HWCAP value right before release OK?
> i need to discuss with linux devs if we can reserve
> the value ahead of time.

glibc would obviously like to see that HWCAP value finalized
and in a released kernel so it doesn't change.
 
> the patch is OK with the current logic, i will try to deal
> with this issue separately.
 
I assume this means you accept the patch as-is?

It's clearer if people provided a "Reviewed-by:" line in cases
like this, that way they indicate a clear intent that the patch
is reviewed and substantial issues solved.

-- 
Cheers,
Carlos.