onetbb: add ppc32 support, fix build for Darwin PPC (updated & rebased) #840
Conversation
|
@vossmjp Could you or someone review this, please? |
There was a problem hiding this comment.
Thanks for your efforts. Overall, I don't see any issues re-adding mac32 related stuff from previous TBB versions and that way support oneTBB with PPC32 as a community contribution. However changes need to be oneAPI compliant (no pre C++11 functionality) and not touch any public headers. I'll provide more feedback later this week.
include/oneapi/tbb/task_group.h
Outdated
| @@ -199,7 +199,9 @@ class task_group_context : no_copy { | |||
| bool reserved5 : 1; | |||
| } my_traits; | |||
|
|
|||
| #if __MAC_OS_X_VERSION_MAX_ALLOWED >= 1070 | |||
There was a problem hiding this comment.
Using this macro all over the place in our public headers is not acceptable. First it looks kind of ugly, and second the platform is not fully supported. We need to find solutions case by case to avoid those changes. What's the problem here? Is the bit_field attribute not supported? This structure should fit into one byte even on 32bit systems.
There was a problem hiding this comment.
@reble It is not obvious what causes the failure, but it fails:
In file included from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.5.0/src/tbb/scheduler_common.h:24,
from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.5.0/src/tbb/task_dispatcher.h:24,
from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.5.0/src/tbb/arena.cpp:17:
/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.5.0/src/tbb/../../include/oneapi/tbb/task_group.h:206:42: error: static assertion failed: Traits shall fit into one byte.
206 | static_assert(sizeof(context_traits) == 1, "Traits shall fit into one byte.");
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~
compilation terminated due to -Wfatal-errors.
I will check on ppc64 a bit later, need to get to another machine.
There was a problem hiding this comment.
@reble Apparently it was failing because size of bool on Darwin PPC is 32 bit: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107590
Macro dropped, setting from 1 to 4 fixes static assert.
include/oneapi/tbb/task_group.h
Outdated
| @@ -417,7 +419,9 @@ class task_group_context : no_copy { | |||
| friend class task_group_base; | |||
| }; // class task_group_context | |||
|
|
|||
| #if __MAC_OS_X_VERSION_MAX_ALLOWED >= 1070 | |||
| static_assert(sizeof(task_group_context) == 128, "Wrong size of task_group_context"); | |||
There was a problem hiding this comment.
here we might have to re calculate the right side of the equation for 32bit/64bit. Not sure if that issue is MAC_OS_X only
There was a problem hiding this comment.
I may need help here :)
For the time-being, I disabled this assert on Darwin PPC, but hopefully we can re-calculate and enable it back.
|
@reble Thank you very much for looking into this! This is greatly appreciated. I am travelling at the moment and do not have access to my PPC machines, so I will return to this ticket by August. (I am really sorry for such a delay, but I just don’t have remote access.) Please keep the ticket opened, and I will reply in detail on every point once I am back and can test the builds. |
|
@reble I am gonna re-do the whole thing now, starting from just adding 32-bit symbols lists to the new version of |
barracuda156
force-pushed
the
onetbb-ppc
branch
from
a6673f1
to
dc25433
Compare
barracuda156
changed the title
|
Test results (10.6 PPC, G5 Quad): |
|
@reble @anton-potapov @alexey-katranov @vossmjp @pavelkumbrasev We really need this fixed now, since Could someone please help me with reviewing this? P. S. Again, please excuse me for taking so long to finalize this, but in result fixes are substantially improved and unnecessary changes are dropped. Also, if someone can advise on fixing remaining tests, would be awesome. Quite likely it is wrong alignments, like Bool is somewhere unconditionally assumed to be 1 byte etc. (it is 4 bytes on Darwin ppc32). |
| bool reserved5 : 4; | ||
| } my_traits; | ||
|
|
||
| static_assert(sizeof(context_traits) == 4, "Traits shall fit into 4 bytes."); |
There was a problem hiding this comment.
I believe whole idea was to pack the traits into one byte. @pavelkumbrasev, please correct me if I'm wrong
There was a problem hiding this comment.
@anton-potapov As it is now in the source, this static assert fails on Darwin ppc32. I believe it is due to bool size being wrong. (I would not have messed with the source unnecessarily – only once it failed, I began looking for a fix.)
https://developer.apple.com/library/archive/documentation/DeveloperTools/Conceptual/LowLevelABI/100-32-bit_PowerPC_Function_Calling_Conventions/32bitPowerPC.html
There was a problem hiding this comment.
IMHO size of bool on Darwin is not related here The code asks compiler to use precisely 1 bit per field of the context_traits structure.
As well gcc 4.8.5 for power can compile this
There was a problem hiding this comment.
@anton-potapov I readily admit I can be wrong regarding a cause, but as a matter of fact existing code fails on macOS PPC:
In file included from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.7.0/src/tbb/scheduler_common.h:24,
from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.7.0/src/tbb/task_dispatcher.h:24,
from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.7.0/src/tbb/arena.cpp:17:
/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.7.0/src/tbb/../../include/oneapi/tbb/task_group.h:202:42: error: static assertion failed: Traits shall fit into one byte.
202 | static_assert(sizeof(context_traits) == 1, "Traits shall fit into one byte.");
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~
compilation terminated due to -Wfatal-errors.
make[2]: *** [src/tbb/CMakeFiles/tbb.dir/arena.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
In file included from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.7.0/src/tbb/scheduler_common.h:24,
from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.7.0/src/tbb/mailbox.h:23,
from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.7.0/src/tbb/arena_slot.h:28,
from /opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.7.0/src/tbb/arena_slot.cpp:17:
/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_devel_onetbb/onetbb/work/oneTBB-2021.7.0/src/tbb/../../include/oneapi/tbb/task_group.h:202:42: error: static assertion failed: Traits shall fit into one byte.
202 | static_assert(sizeof(context_traits) == 1, "Traits shall fit into one byte.");
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~
compilation terminated due to -Wfatal-errors.
There was a problem hiding this comment.
@anton-potapov If it is preferable, we can alternatively not perform that static assert check on macOS ppc32.
There was a problem hiding this comment.
@barracuda156 As an alternative, you might look into GCC's -mone-byte-bool flag. I used this flag to reconcile bool-size issues with mozjs. However, if this is a public header, then client projects will also need that flag passed. (With mozjs I modified the pkg-config file to include the flag.)
There was a problem hiding this comment.
@evanmiller Did not know about that, interesting. I cannot judge however if it is desirable in this case or not.
@iains @anton-potapov @glaubitz @pavelkumbrasev @reble What do you think?
P. S. It certainly does not hurt to try building with this flag and running tests. I will update on that.
There was a problem hiding this comment.
@anton-potapov @glaubitz @pavelkumbrasev @reble @evanmiller So passing -mone-byte-bool as cxxflag indeed removed the need of patching struct context_traits and sizeof(task_group_context). Without the flag correct values are 4 and 136, respectively.
Obviously passing -mone-byte-bool flag simplifies changes, however I do not know if it may cause any adverse effects.
@iains Since this is both Darwin- and GCC-related matter, could you please give an advice? Without looking into specifics (I am aware you are very busy atm), but generally speaking for the flag itself.
There was a problem hiding this comment.
UPD. Hmm, probably not a great choice: while I cannot run the whole testsuite in Rosetta (being away from my PowerMacs), since it freezes, the very beginning is much worse when the build is done with -mone-byte-bool:
/opt/local/bin/ctest --force-new-ctest-process
Test project /opt/local/var/macports/build/_opt_PPCRosettaPorts_devel_onetbb/onetbb/work/build
Start 1: test_tick_count
1/136 Test #1: test_tick_count ..........................***Failed 0.07 sec
Start 2: test_allocators
2/136 Test #2: test_allocators .......................... Passed 0.05 sec
Start 3: test_arena_priorities
3/136 Test #3: test_arena_priorities ....................Bus error***Exception: 0.16 sec
Start 4: test_dynamic_link
4/136 Test #4: test_dynamic_link ........................ Passed 0.04 sec
Start 5: test_collaborative_call_once
5/136 Test #5: test_collaborative_call_once .............***Failed 70.86 sec
Start 6: test_concurrent_lru_cache
6/136 Test #6: test_concurrent_lru_cache ................ Passed 0.04 sec
Start 7: test_concurrent_unordered_map
7/136 Test #7: test_concurrent_unordered_map ............Subprocess aborted***Exception: 4.21 sec
|
Linking against libatomic is also needed on multiple 32-bit targets on Linux. Currently we're explicitly linking against libatomic on mips32, m68k, powerpc32 and sh4 [1]: So, I think 268ada4 should employ a test to determine whether the target platform needs to link against libatomic. For example, the mold linker uses the following trick [2]: # If atomics doesn't work by default, add -latomic.
# We need the flag on riscv, armv6 and m68k.
include(CheckCXXSourceCompiles)
check_cxx_source_compiles("#include <atomic>
int main() {
std::atomic<uint8_t> w1;
std::atomic<uint16_t> w2;
std::atomic<uint32_t> w4;
std::atomic<uint64_t> w8;
return ++w1 + ++w2 + ++w4 + ++w8;
}" HAVE_FULL_ATOMIC_SUPPORT)
if(NOT HAVE_FULL_ATOMIC_SUPPORT)
target_link_libraries(mold PRIVATE atomic)
endif()
|
@glaubitz I have no idea how that works on Linux, to be honest, but on macOS, at least macOS PPC, explicit linking to |
|
@barracuda156 Can you test whether the following works for you: diff --git a/cmake/compilers/GNU.cmake b/cmake/compilers/GNU.cmake
index cd76acfe..8282b54e 100644
--- a/cmake/compilers/GNU.cmake
+++ b/cmake/compilers/GNU.cmake
@@ -44,6 +44,20 @@ if (NOT MINGW)
set(TBB_COMMON_LINK_LIBS dl)
endif()
+include(CheckCXXSourceCompiles)
+check_cxx_source_compiles("#include <atomic>
+int main() {
+ std::atomic<uint8_t> w1;
+ std::atomic<uint16_t> w2;
+ std::atomic<uint32_t> w4;
+ std::atomic<uint64_t> w8;
+ return ++w1 + ++w2 + ++w4 + ++w8;
+}" HAVE_FULL_ATOMIC_SUPPORT)
+
+if(NOT HAVE_FULL_ATOMIC_SUPPORT)
+ set(TBB_COMMON_LINK_LIBS ${TBB_COMMON_LINK_LIBS} atomic)
+endif()
+
# Ignore -Werror set through add_compile_options() or added to CMAKE_CXX_FLAGS if TBB_STRICT is disabled.
if (NOT TBB_STRICT AND COMMAND tbb_remove_compile_flag)
tbb_remove_compile_flag(-Werror)I have successfully tested it on 32-bit PowerPC on Linux. |
@glaubitz Thank you! Allow me a day, I will test it for ppc32 and update you. |
|
@barracuda156 It shouldn't be required on 64-bit machines, only on 32-bit. 64-bit machines can do all the atomic operations in hardware without the need of a helper library. FWIW, it's not a MacOS-related issue, but a GCC issue. There is even a GCC bug for it, see [1]. |
@glaubitz I vaguely recall @iains mentioned elsewhere why GCC behaves that way, but unfortunately forgot related details. |
|
@barracuda156 Well, if you just run a small compile test during cmake stage, you can actually dynamically verify whether a given target needs |
@glaubitz Yes, I agree. I verify and update you soon. P. S. What do you think about static assert case above btw? If any ideas/suggestions. |
ppc64/G5 can indeed do up to 64b lock-free. It needs the library for > 8bytes (again this is an ISA constraint, not a software one).
It seems that there has been some discussion ...
Well. I could have imagined that there would be reluctance to add libraries unilaterally since that puts a burden on code that does not need them (I don't recall reasoning beyond the at the moment) - but let's see what pans out in the PR. |
I agree. But if we actually use a compile test as suggested above this situation will never occur. |
|
@barracuda156 > P. S. What do you think about static assert case above btw? If any ideas/suggestions. Haven't seen this issue on 32-bit PowerPC on Linux. With the libatomic fix, the code builds fine and we have just one failure (#982). |
@glaubitz I have seen size- and alignment-related static assert failures on macOS PPC elsewhere too, these are likely ABI-related and not arch-related as such. (For example, 32-bit Bool is specific to Darwin ppc32.) |
@glaubitz Yes, your suggestion works for macOS PPC. The test fails: And then |
|
@barracuda156 could you please take a look at https://github.com/oneapi-src/oneTBB/blob/master/SYSTEM_REQUIREMENTS.md#limitations |
I think it just takes way too long before PRs get changed. I have a tiny PR fixing testsuite issues on more 32 targets, but that still hasn't been merged: Same for the one to fix atomis: It's not really encouraging to send PRs if they get never merged or discussed ad nauseum. |
Sorry to hear this. The problem is that we don't have the same environment to test such changes. And a ton of question is a only way to come up to product-quality solution. As for such big PR's as this one separate branch would speed-up process significantly. |
There is the GCC compile farm which provides access to a plethora of architectures. Free to use for any open source developer. |
@pavelkumbrasev That is doable, I guess, but I really hope not to end up with being in need of perpetual rebasing against changing master. This already takes forever. ppc32 can be tested in Qemu or natively on Linux or BSD. Darwin-specific fixes: well, maybe you could rely on my testing here? After all, they only affect Darwin PPC, so won’t break anything for anyone, since at the moment it is not supported at all. |
|
The idea with separate branch is: "You keep it in working state". You don't need to rebase this branch every commit in master. You can rebase it once you see something important in master and you have time to rebase and test it. |
|
@pavelkumbrasev That does not solve the issue of eventual merging into master though. What are the reservations against by the way? Maybe we can rather address those, and finally merge this? P. S. We already have this merged into Macports, and I believe @evanmiller may confirm if this builds on 10.5.8 |
The problem is that this patch PR brings changes (non trivial) to source code only to support outdated platform and this is against the https://github.com/oneapi-src/oneTBB/blob/master/SYSTEM_REQUIREMENTS.md#limitations |
|
@pavelkumbrasev This PR has been discussed and worked on for many months with a view that 32-bit macOS support can be restored, and the PR will be merged once polished enough. Nothing of introduced changed adversely affects anything else. If we merge it, everything gonna just work. |
|
So what is your concerns with a separate branch in this repo? |
barracuda156
force-pushed
the
onetbb-ppc
branch
from
16b39df
to
7e5b7c4
Compare
Description
Add a comprehensive description of proposed changes
This PR restores support for Darwin PPC: specifically, Leopard (
ppc,ppc64,universal), 10.6 PPC (10A190) and 10.6.8 Rosetta.UPDATE 2023.02.19:
This should be ready, I guess. Sizes for asserts also fixed. (Alternatively, those two asserts can be disabled for Apple ppc32.)
UPDATE 2022.11.16:
I have re-done everything from scratch using the new
oneTBBversion. There are substantial improvements since the last time:ppc32is now fixed (so all symbols export).GNU.cmake).-Wno-unused-function,-Wno-parentheses. (I did not add this flags, for now, but it can be trivially done, conditioning on CMAKE_OSX_ARCHITECTURES.)There are a couple of spots where macros for macOS version are needed and appropriate, since specific definitions are missing in 10.5 and 10.6 system headers. Those have been minimized.
There is one static assert that fails on PPC – I have conditionally disabled it.
If anyone could improve on my patches, that will be greatly appreciated.
Fixes #819 - issue number(s) if exists
Building
onetbbon Darwin PPC fixed.Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Maybe just mention that Darwin PPC is again supported (as a 2-tier, or whatever it is called).
Breaks backward compatibility
It should restore backward compatibility :)
Notify the following users
List users with
@to send notifications@vossmjp If you could help me to review this PR, it would be awesome!
Other information
My ticket earlier here: #819