Re: [PATCH 1/5] add SWAP macro

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 30, 2017, 20:48 UTC
Message-ID: <alpine.DEB.2.20.1701301806591.3469@virtualbox>
In-Reply-To: <9bcae427-d654-a671-4368-030150168102@web.de>

Hi René,

On Mon, 30 Jan 2017, René Scharfe wrote:

Show 17 quoted lines

> Am 30.01.2017 um 16:39 schrieb Johannes Schindelin:
>
> > On Sat, 28 Jan 2017, René Scharfe wrote:
> >
> > > Add a macro for exchanging the values of variables.  It allows users
> > > to avoid repetition and takes care of the temporary variable for
> > > them.  It also makes sure that the storage sizes of its two
> > > parameters are the same.  Its memcpy(1) calls are optimized away by
> > > current compilers.
> >
> > How certain are we that "current compilers" optimize that away? And
> > about which "current compilers" are we talking? GCC? GCC 6? Clang?
> > Clang 3?  Clang 3.8.*? Visual C 2005+?
> 
> GCC 4.4.7 and clang 3.2 on x86-64 compile the macro to the same object
> code as a manual swap ; see https://godbolt.org/g/F4b9M9.  Both were
> released 2012.

Good. Thank you.

> That website doesn't offer Visual C(++), but since you added the
> original macro in e5a94313c0 ("Teach git-apply about '-R'", 2006) I
> guess we have that part covered. ;)

Back then, I was not able to build Git using Visual C *at all*. It required a Herculean effort to make that happen, and it did not happen before the Git for Windows project was started in 2007.

So I tried to verify that Visual C optimizes this well, and oh my deity, this was not easy. In Debug mode, it does not optimize, i.e. the memcpy() will be called, even for simple 32-bit integers. In Release mode, Visual Studio's defaults turn on "whole-program optimization" which means that the only swapping that is going on in the end is that the meaning of two registers is swapped, i.e. the SWAP() macro is expanded to... no assembler code at all.

After trying this and that and something else, I finally ended up with the file-scope optimized SWAP() resulting in this assembly code:

00007FF791311000 mov eax,dword ptr [rcx] 00007FF791311002 mov r8d,dword ptr [rdx] 00007FF791311005 mov dword ptr [rcx],r8d 00007FF791311008 mov dword ptr [rdx],eax

However, recent events (including some discussions on this mailing list which had unfortunate consequences) made me trust my instincts more. And my instincts tell me that it would be unwise to replace code that swaps primitive types in the straight-forward way by code that relies on advanced compiler optimization to generate efficient code, otherwise producing very suboptimal code.

The commit you quoted embarrasses me, and I have no excuse for it. I would love to see that myswap() ugliness fixed by replacing it with a construct that is simpler, and generates good code even without any smart compiler.

Ciao, Dscho