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.