Re: [boost] [release/atomic] request permission to merge 86144

Subject: Re: [boost] [release/atomic] request permission to merge 86144
From: Gavin Lambert (gavinl_at_[hidden])
Date: 2013-10-14 18:51:15

On 15/10/2013 03:07, Quoth Andrey Semashev:
> I don't think the commit is correct.

I was the original author of the patch, and I can confirm that it is 
correct at least in the fact that DCAS (and other 64-bit atomic 
operations) did not work on VS2008 prior to the patch and does after 
applying it.

I did not test on other platforms or compilers but by inspection the 
changes in the patch should be skipped or otherwise harmless in those cases.

> 1. 64-bit CAS for 32-bit x86 was already supported before. It is controlled by
> the BOOST_ATOMIC_X86_HAS_CMPXCHG8B macro, which is automatically defined when
> the compiler targets Pentium or later (may need to be enabled by a compiler
> switch).

The patch was originally based on Boost 1.53 and 1.54, in which this 
macro is not defined at all.

It does look like better support for 64-bit compare_exchange_* and 
load/store specifically has been added to trunk (and presumably 1.55) 
since then though.

But the patch should still add support for fetch_add, fetch_sub, and 
exchange operations, which still appear to be otherwise unsupported in 
trunk prior to the patch as far as I can tell.

> 2. Your change modifies 64-bit branch, which always have 64-bit atomic ops
> available (i.e. CAS-based path is not needed). This branch is not used by 32-
> bit targets.

No, it's in the #else clause of the 64-bit branch, aka the 32-bit branch.

Probably the changes in interlocked.hpp should now also be wrapped in a 
test for BOOST_ATOMIC_X86_HAS_CMPXCHG8B, since you're right that this 
would be more correct.

But the intrinsic *does* exist on x86 >= Pentium, so defining it here 
means that the assembly code in platform_cmpxchg64_strong is not 
required.  (Having said that, it would probably be harmless to 
completely omit the changes to interlocked.hpp.)

The changes in windows.hpp still seem relevant as noted above.