[5/8] Move i486/pthread_spin_trylock.S to pthread_spin_trylock.S

On Wed, Aug 26, 2015 at 11:02 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 26 Aug 2015, H.J. Lu wrote:
>
>> Since glibc doesn't support i386 any more, we can move
>> i486/pthread_spin_trylock.S to pthread_spin_trylock.S
>
> The i686 version wasn't just a #include - you're losing the HAVE_CMOV
> define in this patch.
>
> This illustrates the use of testing these patches by making sure that
> stripped installed shared libraries (for each of i486, i586 and i686) are
> identical before and after the patch series.
>

Here is the updated patch.  There are no changes in generated
shared libraries for i486, i586 amd i686 targets.

OK for master?

On Wed, 2015-08-26 at 12:33 -0700, H.J. Lu wrote:
> On Wed, Aug 26, 2015 at 11:02 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> > On Wed, 26 Aug 2015, H.J. Lu wrote:
> >
> >> Since glibc doesn't support i386 any more, we can move
> >> i486/pthread_spin_trylock.S to pthread_spin_trylock.S
> >
> > The i686 version wasn't just a #include - you're losing the HAVE_CMOV
> > define in this patch.
> >
> > This illustrates the use of testing these patches by making sure that
> > stripped installed shared libraries (for each of i486, i586 and i686) are
> > identical before and after the patch series.
> >
> 
> Here is the updated patch.  There are no changes in generated
> shared libraries for i486, i586 amd i686 targets.
> 
> OK for master?
> 

Could you instead please try to replace the custom asm implementation
with a C implementation, preferably the generic one in
nptl/pthread_spin_trylock.c?  And, if necessary, and improve the latter
if there is a significant performance difference for uncontended locks?

Thanks!

> Could you instead please try to replace the custom asm implementation
> with a C implementation, preferably the generic one in
> nptl/pthread_spin_trylock.c?  And, if necessary, and improve the latter
> if there is a significant performance difference for uncontended locks?

As I just said in another thread, meaningful changes such as that should
not be conflated with the mechanical directory restructuring HJ is doing
now.  Of course, doing removals first reduces the number of renamings
required.  But the directory restructuring is entirely mechanical and so
its review can consist of verifying that nothing materially changed in the
build whatsoever.  Meaningful changes require meaningful review.

Directory restructuring changes, when generally desireable, should never be
gated on unrelated meaningful changes.  So if HJ wants to do those
meaningful improvements first, that's fine.  But nobody should be objecting
to the directory restructuring changes on the grounds that some other
meaningful change is also desireable.

On 08/26/2015 05:01 PM, Roland McGrath wrote:
>> Could you instead please try to replace the custom asm implementation
>> with a C implementation, preferably the generic one in
>> nptl/pthread_spin_trylock.c?  And, if necessary, and improve the latter
>> if there is a significant performance difference for uncontended locks?
> 
> As I just said in another thread, meaningful changes such as that should
> not be conflated with the mechanical directory restructuring HJ is doing
> now.  Of course, doing removals first reduces the number of renamings
> required.  But the directory restructuring is entirely mechanical and so
> its review can consist of verifying that nothing materially changed in the
> build whatsoever.  Meaningful changes require meaningful review.
> 
> Directory restructuring changes, when generally desireable, should never be
> gated on unrelated meaningful changes.  So if HJ wants to do those
> meaningful improvements first, that's fine.  But nobody should be objecting
> to the directory restructuring changes on the grounds that some other
> meaningful change is also desireable.

+1

c.

On Wed, 2015-08-26 at 14:01 -0700, Roland McGrath wrote:
> > Could you instead please try to replace the custom asm implementation
> > with a C implementation, preferably the generic one in
> > nptl/pthread_spin_trylock.c?  And, if necessary, and improve the latter
> > if there is a significant performance difference for uncontended locks?
> 
> As I just said in another thread, meaningful changes such as that should
> not be conflated with the mechanical directory restructuring HJ is doing
> now.  Of course, doing removals first reduces the number of renamings
> required.  But the directory restructuring is entirely mechanical and so
> its review can consist of verifying that nothing materially changed in the
> build whatsoever.  Meaningful changes require meaningful review.
> 
> Directory restructuring changes, when generally desireable, should never be
> gated on unrelated meaningful changes.  So if HJ wants to do those
> meaningful improvements first, that's fine.  But nobody should be objecting
> to the directory restructuring changes on the grounds that some other
> meaningful change is also desireable.
> 

I don't disagree with that -- I simply hadn't paid attention to the
wider set of changes, and moving the file around seemed like a good
opportunity to try to get rid of custom asm.

> I don't disagree with that -- I simply hadn't paid attention to the
> wider set of changes, and moving the file around seemed like a good
> opportunity to try to get rid of custom asm.

It is indeed a good reminder to look at such files and think about removing
them.  If you proposed patches to remove some such files right now, they
would probably get consensus very quickly.