Bug 34106 - [parallel mode] Atomic operations compatibility layer needs cleanup
Summary: [parallel mode] Atomic operations compatibility layer needs cleanup
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.3.0
: P3 normal
Target Milestone: 4.8.0
Assignee: Jonathan Wakely
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-15 10:09 UTC by Johannes Singler
Modified: 2012-09-29 18:08 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2007-11-15 10:11:16


Attachments
remove support for other compilers (2.49 KB, patch)
2009-03-11 14:29 UTC, Jonathan Wakely
Details | Diff
use BITS_PER_UNIT instead of hardcoded 8 (338 bytes, patch)
2009-06-22 20:25 UTC, Jonathan Wakely
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johannes Singler 2007-11-15 10:09:45 UTC
In compatibility.h, there are a lot of compiler- and architecture dependent switches. Relying on the GCC atomic operations would make this much cleaner.
Comment 1 Jonathan Wakely 2009-03-11 14:29:16 UTC
Created attachment 17438 [details]
remove support for other compilers

this patch re-implements the parallel mode's atomic operations in terms of the GCC builtins
Comment 2 Jonathan Wakely 2009-06-22 20:25:35 UTC
Created attachment 18049 [details]
use BITS_PER_UNIT instead of hardcoded 8

additional patch to use BITS_PER_UNIT for lcas_t_bits
Comment 3 Paolo Carlini 2009-06-22 20:48:02 UTC
Ok, we are very far from having the parallel mode facilities correctly uglified, but we could as well use __CHAR_BIT__...
Comment 4 Paolo Carlini 2010-01-28 17:28:34 UTC
Note: I don't think we should remove support for ICC, since lately it doesn't come anymore with Dinkum, libstdc++-v3 is the default C++ runtime. Likewise, we should probably keep support for mingw32 and cygwin. In short I think we should be careful with the sort of clean up envisaged by Jon in Comment #1.
Comment 5 Jonathan Wakely 2010-01-28 17:46:10 UTC
and I thought everyone had forgotten about that patch ;)

granted, ICC uses libstdc++, but doesn't it already have to support the same atomic builtins as used elsewhere in the library?  And my patch changes parallel mode to use those same builtins (which are designed after the Intel intrinsics anyway, so likely to work in ICC!)
Comment 6 Jonathan Wakely 2010-01-28 17:47:41 UTC
similarly, if cygwin and mingw32 implement __sync_fetch_and_add etc. then why keep custom versions for those platforms?  (but maybe the builtins aren't implemented on those platforms - I have no idea)
Comment 7 Paolo Carlini 2010-01-28 17:51:30 UTC
I don't know the details frankly, but at minimum we should be *very* careful and make sure we do the right thing also in the special cases, eg, on i386 when the atomic builtins are not available.
Comment 8 Jonathan Wakely 2012-09-27 17:37:46 UTC
The compatibility.h header is still a complete mess with code we don't need, can I take this bug and clean it up?

(In reply to comment #4)
> Note: I don't think we should remove support for ICC, since lately it doesn't
> come anymore with Dinkum, libstdc++-v3 is the default C++ runtime.

But the code guarded by __ICC uses Win32 API functions, so can only work on Windows, not when ICC is used with libstdc++

With the new __atomic built-ins the file should be even simpler.
Comment 9 Paolo Carlini 2012-09-27 17:39:28 UTC
Definitely Jon!
Comment 10 Jonathan Wakely 2012-09-27 17:53:12 UTC
mine then
Comment 11 Jonathan Wakely 2012-09-29 17:58:42 UTC
Author: redi
Date: Sat Sep 29 17:58:34 2012
New Revision: 191856

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191856
Log:
	PR libstdc++/34106
	* include/parallel/compatibility.h: Remove non-GCC code.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/parallel/compatibility.h
Comment 12 Jonathan Wakely 2012-09-29 18:08:23 UTC
Non-GCC code removed, so closing as fixed.

See Bug 54754 for further changes needed to support non-x86_64 platforms.