Opened 9 years ago

Closed 9 years ago

#9426 closed Patches (fixed)

boost::atomic gcc-cas related specializations patch

Reported by: Piotr Kobzda <pikob@…> Owned by: timblechmann
Milestone: To Be Determined Component: atomic
Version: Boost 1.55.0 Severity: Problem
Keywords: Cc: Andrey.Semashev@…

Description

Patch adds BOOST_ATOMIC_ALLOW_GCC_CAS library configuration option causing preferred use of atomic implementation specializations from gcc-cas.hpp instead of default ones.

Patch includes also all changes necessary to fix compilation problems of gcc-cas.hpp, and cas32strong.hpp.

Tested with gcc 4.5.3 (and few earlier versions) on ARM (manually forced) and on mipsel platforms.

The gcc-cas atomic specializations (full memory barrier based) appeared to perform better on platforms where the default ones where used (pthread mutex based ones).

However, the original author has disabled gcc-cas specialization with the following comment:

/* currently does not work correctly */

which is a good reason for making use of it optional so far.

Anyway, it would be great to know the details of the problems related the above comment, which could help in deciding if a new option can be used (and possibly help in solving of the issue).

Attachments (2)

boost_1_55_0_atomic_gcc_cas.patch (4.3 KB ) - added by Piotr Kobzda <pikob@…> 9 years ago.
boost_1_55_0_atomic_gcc_cas_no_opt.patch (4.3 KB ) - added by Piotr Kobzda <pikob@…> 9 years ago.

Download all attachments as: .zip

Change History (13)

by Piotr Kobzda <pikob@…>, 9 years ago

comment:1 by timblechmann, 9 years ago

unfortunately the original author disappeared and i have no idea what's the problem with the original code. could be a compiler bug which may or may not have been fixed ... in general, the patch looks ok to me, though...

comment:2 by Andrey Semashev, 9 years ago

I welcome the compilation fixes for the gcc-cas.hpp and cas32strong.hpp headers but I don't see the point in the configuration macro. The ARM target should use the implementation from gcc-armv6plus.hpp (not lock-based). If it doesn't then that's probably a bug in the target detection in platform.hpp and I'd prefer it to be fixed instead. gcc-cas.hpp should be included only since gcc 4.1, because __sync* intrinsics were not available before. It should also check if __GCC_HAVE_SYNC_COMPARE_AND_SWAP_N macros are defined. I suppose, that implementation would be used in case of MIPS. In any case, there is no need in forcing gcc-cas.hpp, unless I'm missing something.

PS: I'm not aware of any hidden problems with gcc-cas.hpp. When it's compilation problems are fixed I'm in favor of enabling it in platform.hpp.

comment:3 by Andrey Semashev, 9 years ago

Cc: Andrey.Semashev@… added

comment:4 by timblechmann, 9 years ago

iirc the __GCC_HAVE_SYNC macros have been introduced with a later gcc version, so it is probably a good idea only to use the gcc-cas version when these macros are available.

regaring arm, it may actually be a good idea to use gcc-cas on armv7, because it provides a double-width ll/sc, which is currently not supported by boost.atomic (but quite useful for many lock-free algorithms). or implement the complete arm support in assembly to avoid the ll/sc emulating cas emulating atomics issue ...

comment:5 by Andrey Semashev, 9 years ago

or implement the complete arm support in assembly to avoid the ll/sc emulating cas emulating atomics issue

That was my idea, yes.

comment:6 by Piotr Kobzda <pikob@…>, 9 years ago

Just to make it clear, my patch was not ARM targeted (nor any other possible target that uses default atomics impl. currently) -- I just used ARM to verify gcc-cas implementation correctness. I guess that current selection for ARMs is usually better than gcc-cas (at least for ARMs 6+). My primary intent was to allow use of gcc-cas on MIPSel platforms on which pthread mutexes are currently being selected by default. BTW, I'd prefer to use lower level APIs for that, but currently I can not use nothing better then generic atomics for Mips implemented based on strong memory barrier only -- which most likely gives no noticeable advantages over implementation from gcc-cas.

Regarding optional use of gcc-cas, if you think, that unconditional use of it (besides of earlier checks from platform.hpp and additional check for minimum gcc version supporting __sync API of course) is safe enough and better than default selection, I'd also opt for that.

comment:7 by tim@…, 9 years ago

piotr: not being familiar with mipsel toolchains, i wonder: what (gcc) compilers are commonly used for this architecture? e.g. is it common to use gcc older than 4.4? iirc 4.3 or 4.4 started to define the __GCC_HAVE_SYNC macros ... and tbo i'd rather not spend too much time on toolchains which are hardly used these days (e.g. gcc-3.X) or the like ...

andrey: real native arm support would be great, but also here i wonder: is it really worth to spend time on this? i suppose that most arm toolchains provide std::atomic these days...

comment:8 by Andrey Semashev, 9 years ago

real native arm support would be great, but also here i wonder: is it really worth to spend time on this? i suppose that most arm toolchains provide std::atomic these days...

Well, I'd like Boost.Atomic to be at least not worse than std::atomic on all supported targets. If Boost.Atomic offers additional features, such as specialized operations, this may be a good reason to prefer it over std::atomic.

in reply to:  7 comment:9 by Piotr Kobzda <pikob@…>, 9 years ago

not being familiar with mipsel toolchains, i wonder: what (gcc) compilers are commonly used for this architecture?

Hard to tell precisely, since it depends on strategy of the processor vendors adopted to theirs individual customers (which, in general, I'm not too aware of). But I'm afraid, that some people can still use toolchains for Mips older than 4.1 (which I think is initial version supporting __sync_synchronize builtin). Nevertheless, I agree that having just gcc version check would most likely be enough here -- anyone interested in using of older toolchains will still safely use default alternative.

I'm just not sure, whether suggested use of __GCC_HAVE_SYNC_* macros shall be preferred before simple gcc version check -- official documentation for gcc 4.1 does not list it in predefined macros list, thus I'd rather prefer use of version check.

by Piotr Kobzda <pikob@…>, 9 years ago

comment:10 by Piotr Kobzda <pikob@…>, 9 years ago

Added patch that checks gcc version (4.1+) instead of use of BOOST_ATOMIC_ALLOW_GCC_CAS option before inclusion of gcc-cas.hpp.

comment:11 by timblechmann, 9 years ago

Resolution: fixed
Status: newclosed

applied the patch to git

Note: See TracTickets for help on using tickets.