Re: [PATCH] Remove Prefer_SSE_for_memop on x64

[Andreas Jaeger <aj at suse dot com>]

On 03/07/2013 05:20 PM, OndÅej BÃlka wrote:
> On Thu, Mar 07, 2013 at 10:12:27AM +0100, Andreas Jaeger wrote:
> > On 02/14/2013 10:30 PM, OndÅej BÃlka wrote:
> > > Hello,
> > > As I asked in
> > > http://www.sourceware.org/ml/libc-alpha/2013-02/msg00203.html
> > > nobody objected to keep non-sse memset on x64.
> > >
> > > This patch removes that implementation and flag Prefer_SSE_for_memop
> > > which is always set and only choose sse memset over nonsse one.
> > >
> > > 2013-02-14   OndÅej BÃlka  <neleai@seznam.cz>
> > >
> > > 	* sysdeps/x86_64/memset.S: Always define bcopy.
> >
> > I suggest:
> > Remove USE_MULTIARCH conditional for definining bcopy.
>
> I will remove that.

You did not change the entry at all.

> > > 	* sysdeps/x86_64/multiarch/Makefile: Update.
> >
> > Which variable are you changing - and how?

Seems you did not understand what I asked for.

We mention the name of the variable changed and comment on it, e.g.

* sysdeps/x86_64/multiarch/Makefile (sysdep_routines): Remove memset-x86-64.

> > > 	* sysdeps/x86_64/multiarch/ifunc-impl-list.c: Update.
> > > 	* sysdeps/x86_64/multiarch/init-arch.c: Remove Prefer_SSE_for_memop.
> >
> > The two entries above miss the function changed. Replace Update with
> > a proper description.
> >
> > > 	* sysdeps/x86_64/multiarch/init-arch.h: Likewise.
> >
> > Mention every single define your remove.
> >
> > > 	* sysdeps/x86_64/multiarch/bzero.S: Remove.
> > > 	* sysdeps/x86_64/multiarch/memset-x86-64.S: Likewise.
> > > 	* sysdeps/x86_64/multiarch/memset.S: Likewise.
> > > 	* sysdeps/x86_64/multiarch/memset_chk.S: Likewise.
> >
> > The patch itself is fine, please send an updated changelog for review,
> >
> > thanks,
> > Andreas
> Here is updated changelog
>
> 	* sysdeps/x86_64/memset.S: Always define bcopy.
>
> 	* sysdeps/x86_64/multiarch/init-arch.c: Remove
> 	Prefer_SSE_for_memop.

This is done in function __init_cpu_features, so add it:
	* sysdeps/x86_64/multiarch/init-arch.c (__init_cpu_features): Remove 
Prefer_SSE_for_memop.

> 	* sysdeps/x86_64/multiarch/init-arch.h: Remove
>    bit_Prefer_SSE_for_memop, index_Prefer_SSE_for_memop,
> 	HAS_PREFER_SSE_FOR_MEMOP.


> 	
> 	* sysdeps/x86_64/multiarch/bzero.S: Remove.

Let's just say Remove file so that it's clear.

> 	* sysdeps/x86_64/multiarch/memset-x86-64.S: Likewise.
> 	* sysdeps/x86_64/multiarch/memset.S: Likewise.
> 	* sysdeps/x86_64/multiarch/memset_chk.S: Likewise.
>
> 	* sysdeps/x86_64/multiarch/Makefile: Do not compile memset-x86-64.

See above for my suggestion.
> 	 * sysdeps/x86_64/multiarch/ifunc-impl-list.c (bzero, memset):
>    Remove ifunc

This is function __libc_ifunc_impl_list, so the entry should be 
something like:
* sysdeps/x86_64/multiarch/ifunc-impl-list.c (__libc_ifunc_impl_list): 
Remove bzero, memset ifunc support.

Please update the changes entry and repost it with the complete patch,

Andreas
--
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 NÃrnberg, Germany
   GF: Jeff Hawn,Jennifer Guild,Felix ImendÃrffer,HRB16746 (AG NÃrnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126