[release/atomic] request permission to merge 86144
i'd like to merge r86144 into release. it is not exactly a bugfix, but it enables double-width cas support for msvc, which is rather important for many lock-free algorithms to be lockfree with this toolchain (including some configurations of stack/queue of boost.lockfree). thnx, tim
On Monday 14 October 2013 15:49:46 Tim Blechmann wrote:
i'd like to merge r86144 into release. it is not exactly a bugfix, but it enables double-width cas support for msvc, which is rather important for many lock-free algorithms to be lockfree with this toolchain (including some configurations of stack/queue of boost.lockfree).
I don't think the commit is correct. 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). 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.
i'd like to merge r86144 into release. it is not exactly a bugfix, but it enables double-width cas support for msvc, which is rather important for many lock-free algorithms to be lockfree with this toolchain (including some configurations of stack/queue of boost.lockfree).
I don't think the commit is correct.
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).
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.
ccing gavin, who provided the patch and asked me to merge it.
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.
Mere moments ago, quoth I:
On 15/10/2013 03:07, Quoth Andrey Semashev:
I don't think the commit is correct. [...] 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.
To rephrase this slightly: the patch and commit as current should
improve behaviour while doing no harm (except possibly when compiling
for < Pentium, but MSVC doesn't let you do that any more anyway).
It probably could be further improved on, given other changes made to
trunk in the meantime, however (such as closing that
On Tuesday 15 October 2013 12:05:55 Gavin Lambert wrote:
To rephrase this slightly: the patch and commit as current should improve behaviour while doing no harm (except possibly when compiling for < Pentium, but MSVC doesn't let you do that any more anyway).
It probably could be further improved on, given other changes made to trunk in the meantime, however (such as closing that
Boost.Atomic should be working for 486 as well, and MSVC (at least, older versions) have a switch to enable that target. I think, it was actually the default on some older MSVC, but I'm not sure.
On Tuesday 15 October 2013 11:51:15 Gavin Lambert wrote:
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.
The support for 64-bit CAS on 32-bit x86 was indeed added after 1.54 was released (the initial commit that adds the support was 84695). After that the patch was not needed. The support should be tested by our testers since then, so I'm quite sure it should be working. However, I would appreciate if you tried out the current trunk sans the commit 86144 to verify it works.
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.
I understand the patch might be helpful for these releases, and it might have been correct for that code. But the patch was eventually applied to trunk, which has a very different code now. It was a very wrong thing to do - to blindly apply the patch to the code it was not intended to. I wonder how it even applied without complaining.
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.
No, the trunk code before 86144 had support for all 64-bit operations, both for 64 and 32-bit targets. In the latter case, they were implemented through DCAS (see atomic/detail/cas64strong.hpp).
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.
Are we looking at the same code? The change to windows.hpp starts at line 1313, which is inside the "#if defined(_M_AMD64) || defined(_M_IA64)" block. The rest of the changes are also within this block. I'm looking at the current trunk now.
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.
That macro is not defined when interlocked.hpp is included. The change to interlocked.hpp is not needed because windows.hpp doesn't use the _InterlockedCompareExchange64 intrinsic to implement cmpxchg8b.
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.
Given the above, I don't think so. I'm in favor of reverting that commit.
On 15/10/2013 13:06, Quoth Andrey Semashev:
However, I would appreciate if you tried out the current trunk sans the commit 86144 to verify it works.
That could be awkward for me at present, but I'll see if I can try the 1.55 beta build, which should have the same effect as that commit hasn't been merged through yet.
No, the trunk code before 86144 had support for all 64-bit operations, both for 64 and 32-bit targets. In the latter case, they were implemented through DCAS (see atomic/detail/cas64strong.hpp).
Ah yes, I see now. I don't have the trunk code downloaded at the moment so I'm looking at it through an SVN browser peephole, and it's easy to miss details in other files.
Are we looking at the same code? The change to windows.hpp starts at line 1313, which is inside the "#if defined(_M_AMD64) || defined(_M_IA64)" block. The rest of the changes are also within this block. I'm looking at the current trunk now.
Sorry, I thought you were referring to the interlocked.hpp changes. You're right, it looks like the windows.hpp changes have been misapplied, due to the changes between 1.54 and 1.55/trunk. (They're not *completely* useless, as it still defines a fallback path in case eg. BOOST_ATOMIC_INTERLOCKED_EXCHANGE_ADD64 is not defined on x64 for some reason, but as currently that can't happen it's a needless redundancy.)
That macro is not defined when interlocked.hpp is included. The change to interlocked.hpp is not needed because windows.hpp doesn't use the _InterlockedCompareExchange64 intrinsic to implement cmpxchg8b.
I noticed that, although I'm not entirely sure why it doesn't. Wouldn't using the intrinsic be simpler than using the assembly code directly? It probably doesn't really matter in the end though, the effect should be the same.
Given the above, I don't think so. I'm in favor of reverting that commit.
I agree with that now. I was not aware of the 1.55 changes at the time I made the patch.
On Tuesday 15 October 2013 13:43:57 Gavin Lambert wrote:
On 15/10/2013 13:06, Quoth Andrey Semashev:
However, I would appreciate if you tried out the current trunk sans the commit 86144 to verify it works.
That could be awkward for me at present, but I'll see if I can try the 1.55 beta build, which should have the same effect as that commit hasn't been merged through yet.
Yes, 1.55 beta will do as well.
You're right, it looks like the windows.hpp changes have been misapplied, due to the changes between 1.54 and 1.55/trunk. (They're not *completely* useless, as it still defines a fallback path in case eg. BOOST_ATOMIC_INTERLOCKED_EXCHANGE_ADD64 is not defined on x64 for some reason, but as currently that can't happen it's a needless redundancy.)
There is no such configuration, on 64-bit targets all 64-bit intrinsics are available.
That macro is not defined when interlocked.hpp is included. The change to interlocked.hpp is not needed because windows.hpp doesn't use the _InterlockedCompareExchange64 intrinsic to implement cmpxchg8b.
I noticed that, although I'm not entirely sure why it doesn't. Wouldn't using the intrinsic be simpler than using the assembly code directly? It probably doesn't really matter in the end though, the effect should be the same.
The intrinsic is not supported by MSVC 7.1.
participants (3)
-
Andrey Semashev -
Gavin Lambert -
Tim Blechmann