Re: [PATCH 1/3] powerpc: remove power6 wcscpy optimization

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

On 11/03/2019 16:04, Gabriel F. T. Gomes wrote:
> On Mon, Mar 11 2019, Adhemerval Zanella wrote:
>>
>> Yes, I am aware this is a performance regression on power. Specifically
>> for wcscpy it would require 3 call functions call: wcslen follower by
>> wmemcpy and memcpy (and it is worse on powerpc64le abi which does not
>> allow tail-cail optimization).
>>
>> The main question here is should we really care to optimize wcs* 
>> routines at all? 
> 
> This question is two-fold.  On the one hand, and based on the usage
> analysis you provided below, I tend to agree that we shouldn't spend
> time trying to optimize them.  On the other hand, the optimization is
> already in place, so I don't see a compelling reason to remove it.
> 
> I get it that the benefit of removing such optimizations is easier code
> maintenance, but the events of problems seem to be rare enough that
> fixing them when they show up, is something I would be willing to do
> (assuming that that is really the only benefit of the code removal).
> Maybe I'll regret it in the future, but then I could change my mind :).

It is more in term of the current goals of making glibc as simple of
possible, while still retaining optimizations where they *matter*. As I
said we used to make up for lack of compiler support on math and string
optimizations and current trend is to avoid and remove such complications.

Same idea is for a lot of assembly optimization, where a more clean C
implementation usually does not only pays of for simplicity, but sometimes
might also shows better performance with better compiler support.

>  
>>   - Mostly uses I could find on https://codesearch.debian.net are for
>>     Win32 routines (which uses UTF16 as default).
>>
>>   - gnulib define the wchar module as obsolete and even on the project
>>     that does it, it is tied to windows routines.
>>
>>   - wide char routine are inherit problematic regarding portability
>>     (standard does not define wchar_t size).
>>
>>   - Although some runtime environments might use UTF16 as default
>>     (JavaScript, java, Qt, Python), afaik they do not rely on C runtime
>>     exactly for the previous issue and they reimplement all string
>>     routines internally.
>>
>>   - Mostly uses are not performance extensive.
> 
> Thanks for doing this research.
>  
>> Besides the issues described with wcs* routines my main issue with 
>> powerpc optimization is I really think we should avoid that kind of
>> optimizations that try to leverage bad compiler code generation or
>> lack of optimization. Our recent move is to try to leverage the 
>> compiler itself (take for instance the internal math.h refactoring
>> we did). 
> 
> OK, but at the same time, this discussion is also about the fact that we
> don't want to spend time to optimize wcs* functions any *further*.  One
> of the arguments for the removal of the code is that the compiler should
> provide such optimization, however, it currently doesn't.

No, the argument is such optimization should *not* in libc without a good
reason and we should aim to first improve generic implementations and only
push to arch-specific implementation when generic code is not a 
straightforward gain on all architectures (with strategies such as unaligned
access, prefetch, etc.).

> 
>> So what is really the point of still providing such complexity for
>> powerpc for routines that are most likely not used in realworld cases?
> 
> I can't say that there aren't realworld cases, even though it seems to
> be the case in the research you did, because we don't have access to
> everybody's software.
> 
> Anyhow, we, as community, still don't know if we want wcs* functions to
> be optimized, but if we decide that we don't want platform-specific
> optimizations in glibc, then this should be done globally, not for
> powerpc alone.  Right now, this patch only impacts powerpc with a
> performance degradation.
> 
> The only benefit of removing the optimization is to ease code
> maintenance/complexity.  I'm not convinced that it justifies the
> performance impact.
> 

My point if this kind of code would be proposed nowadays we would probably
make it generic instead of pushing for arch-specific code.  In any case 
I think we can towards in not hurting powerpc performance while still
simplifying the code by adding the loop unroll in generic implementation
and making powerpc unroll by default. I still think this complexity is
still practically unnecessary, but better than what we have today.