Re: [PATCH] powerpc: Use aligned stores in memset

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

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> I think one way to provide a slight better memcpy implementation for POWER8
> and still be able to circumvent the non-aligned on non-cacheable memory
> is to use tunables.
>
> The branch azanella/memcpy-power8 [1] has a power8 memcpy optimization which
> uses unaligned load and stores that I created some time ago but never actually
> send upstream.  It shows better performance on both bench-memcpy and
> bench-memcpy-random (about 10% on latter) and mixed results on bench-memcpy-large
> (which it is mainly dominated by memory throughput and on the environment I am
> using, a shared PowerKVM instance, the results does not seem to be reliable).
>
> It could use some tunning, specially on some the range I used for unrolling
> the load/stores and it also does not care for unaligned access on cross-page
> boundary (which tend to be quite slow on current hardware, but also on
> current page size of usual 64k also uncommon).
>
> This first patch does not enable this option as a default for POWER8, it just
> add on string tests as an option.  The second patch changes the selection to:
>
>   1. If glibc is configure with tunables, set the new implementation as the
>      default for ISA 2.07 (power8).
>
>   2. Also if tunable is active, add the parameter glibc.tune.aligned_memopt
>      to disable the new implementation selection.

I think it would be safer if we don't change the default behavior.
IMHO, programs that want a performance improvement would have to set a
tunable.

In other words, the new implementation would be disabled by default.

> So programs that rely on aligned loads can set:
>
> GLIBC_TUNABLES=glibc.tune.aligned_memopt=1

I also think that we should not expose internal details of the implementation
to users, i.e. avoiding to use aligned/unaligned in the name of the function
and in the tunables.
I think that glibc.tune.cached_memopt=1 better exposes what is the optimal
use-case scenario of this implementation.

> This is a RFC patch and if the idea sounds to powerpc arch mantainers I can
> work on finishing the patch with more comments and send upstream.  I tried
> to apply same unaligned idea for memset and memmove, but I could get any real
> improvement in neither.

I like the idea.  Could you merge both patches and send it to libc-alpha,
please?

> [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/memcpy-power8
> [ bench-memcpy-random.out: text/plain ]
> {
>  "timing_type": "hp_timing",
>  "functions": {
>   "memcpy": {
>    "bench-variant": "random",
>    "ifuncs": ["__memcpy_power8",

I also suggest give a more specific name, e.g. __memcpy_power8_cached.
That would make room for a POWER8 implementation what uses only naturally
aligned loads/stores.

Your implementation uses lxvd2x and stxvd2x, which should be avoided in a
cache-inhibited scenario, i.e. glibc.tune.aligned_memopt=0.

However, after changing the tunables' name to glibc.tune.cached_memopt, I think
these instructions could stay they're executed when glibc.tune.cached_memopt=1.

Thanks!!!

-- 
Tulio Magno