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

[Adhemerval Zanella]

On 09/03/2019 13:08, Gabriel F. T. Gomes wrote:
> On Mon, Mar 04 2019, Adhemerval Zanella wrote:
>> This patch removes the power6 wcscpy optimization.  It is rarely
>> int real word codo, the optimizations can be potentially implemented
>> in generic code (which would also benefit not only powerpc), and
>> the power6 and power7 resulting binary are essentially the same.
> 
> Although I agree that optimizations could be implemented in generic
> code, and that it might benefit other architectures, I tested this patch
> and it causes large performance regressions in benchtests, when ran on
> Power8 and Power9 machines (Power 7 numbers were OK).  Based on that
> result (which I shall present and discuss later in this message), I
> think this patch should not be committed to master.
> 
> I am aware that this patch has the additional benefit of easing code
> maintenance, however, the performance impact on Power would be seen
> immediately, while the benefit of an optimization to the generic
> algorithm would need more time to implement.  Thus, I suggest that this
> patch be delayed until we have such implementation.
> 
> 
> Now, regarding the results, I have attached a few spreadsheets, each
> containing the raw data collected from 'make bench' runs on different
> machines, as well as comparisons between the master branch and another
> branch with this patch applied.
> 
>   attachment/wcscpy-LE-P9.ods: Runs on a Power 9, little-endian machine.
>   attachment/wcscpy-LE-P8.ods: Runs on a Power 8, little-endian machine.
>   attachment/wcscpy-BE-P8.ods: Runs on a Power 8, big-endian machine.
>   attachment/wcscpy-BE-P7.ods: Runs on a Power 7, big-endian machine.
> 
> In each of these files, each sheet is named according to the data it
> contains: for instance, 'master - cpu=p8' contains the raw data
> collected from a build of the master branch, which has been configured
> with --with-cpu=power8.  Likewise, 'patched - cpu=p8' contains similar
> data, but from a build of a branch with this patch applied.  Finally, a
> sheet named 'compare p8' contains the performance comparison between the
> two.  In such 'compare pN' sheets, I added a graph of the performance
> comparison of the two, in which the data is sorted from worst to best
> case, so that we can have a visual assessment of how good or bad the
> change is for the whole set of inputs tested (I believe that this kind
> of plot is common practice, but I'm not sure, hence this explanation).
> 
> In wcscpy-BE-P8.ods, wcscpy-LE-P8.ods, and wcscpy-LE-P9.ods, we can see
> that, for allmost all inputs, there is a performance regression (dots
> below 0% are regressions).  On wcscpy-BE-P7.ods, there are more
> performance gains than regressions, and I would be OK with this patch
> being applied to master, if it weren't for the P8 and P9 results.
> 
> PS: I have not yet tested the other patches in this patch set.  This
> message is only about this specific patch.
> 

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? 

  - 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.

The architectures that does provide optimized routine for it (x86
and s390) seems to do so only to advertise the architecture instruction
set than to focus on realword cases.  

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). Also, if I recall correctly I tried to add a loop unrolling
on strcasecmp on generic that was refused exactly because such
optimization should *not* be in generic implementation (which I tend
to agree).

So what is really the point of still providing such complexity for
powerpc for routines that are most likely not used in realworld cases?
If powerpc performance for this routines are really demanded I think
I can thinker some macro to unroll the generic routines for power only.
This will still show some performance hit on wcscpy (mostly the function
call overhead).