-
Notifications
You must be signed in to change notification settings - Fork 407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Darwin 32-bit and PPC #5916
Conversation
Can one of the admins verify this patch? |
Please issue your pull request against the |
@dalg24 Got it, will be done. |
UPD. False alarm, I just needed to switch base to |
Current result with tests:
Skipping works as supposed:
The single failure is the following:
|
One thing which is also necessary and not yet added into CMakeLists is linking to |
Regarding the two failing tests: can we in 32bit mode simply adjust the test size to not use more than 2G memory? |
Also does github offer any type of 32bit testing we could enable for our CI? |
@crtrott I do not know how to do that :) |
I don't think there are any 32bit platforms available but we could create 32bit executables using |
IMO for the most purposes that will do. I do half of my testing for ppc32 on 10.6.8 x86_64. This trick won’t work on other OS, but i386 should be good, as long as OS permits running binaries (macOS from Catalina onward won’t). |
@masterleinad can you try add a build with -m32 to this and also maybe help rebase the thing so we could get this moving? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks look overall.
Please print something in Kokkos::print_configuration in 32-bit mode.
containers/unit_tests/TestVector.hpp
Outdated
@@ -51,8 +51,11 @@ struct test_vector_insert { | |||
it += 17; | |||
it_return = a.insert(it, n + 5, scalar_type(5)); | |||
|
|||
using difference_type = | |||
typename std::iterator_traits<decltype(it)>::difference_type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any good reason not to use std::ptrdiff_t
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this setup is becoming too complex
Did you consider having the 32-bit build in a separate workflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can try that.
Retest this please. |
Retest this please |
failed twice in a row. |
Did we make any changes for CUDA here? Should not be relevant for 32 bit, perhaps. |
@dalg24 I just ran the test manually 30 times in a row in |
- name: Configure Kokkos | ||
run: | | ||
cmake -B builddir \ | ||
-DCMAKE_INSTALL_PREFIX=/usr \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default what is the point in specifying?
Also we don't even install. Please drop that option.
-Ddesul_ROOT=/usr/desul-install/ \ | ||
-DKokkos_ENABLE_DESUL_ATOMICS_EXTERNAL=OFF \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why specifying desul root directory and then disable finding it and use the bundled version?
-DKokkos_ENABLE_TESTS=ON \ | ||
-DKokkos_ENABLE_BENCHMARKS=ON \ | ||
-DKokkos_ENABLE_EXAMPLES=ON \ | ||
-DKokkos_ENABLE_LIBDL=OFF \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why disable LIBDL?
- name: Checkout desul | ||
uses: actions/checkout@v3 | ||
with: | ||
repository: desul/desul | ||
ref: 477da9c8f40f8db369c28dd3f93a67e376d8511b | ||
path: desul | ||
- name: Install desul | ||
working-directory: desul | ||
run: | | ||
git submodule init | ||
git submodule update | ||
mkdir build | ||
cd build | ||
cmake -DDESUL_ENABLE_TESTS=OFF -DCMAKE_INSTALL_PREFIX=/usr/desul-install .. | ||
sudo cmake --build . --target install --parallel 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not convinced it was necessary to use an external desul atomics for that CI build but it is not even used below. Please remove.
-DCMAKE_BUILD_TYPE=RelWithDebInfo | ||
- name: Build | ||
run: | | ||
ccache -z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ccache is not actually used as the compiler launcher in the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go!
-DKokkos_ENABLE_TESTS=ON \ | ||
-DKokkos_ENABLE_BENCHMARKS=ON \ | ||
-DKokkos_ENABLE_EXAMPLES=ON \ | ||
-DKokkos_ENABLE_LIBDL=ON \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have just omitted. Feel free to ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go!
Only |
Thanks to everyone for working on this! |
Fixes #5769
Test suite almost passes:
@crtrott @PhilMiller Please take a look.