Skip to content
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

Made TBB optional for hipstdpar tests #507

Merged

Conversation

NB4444
Copy link
Contributor

@NB4444 NB4444 commented Dec 23, 2024

Hipstdpar should work with and without TBB, so made a flag that as a default is OFF to make use or not of TBB.
Also set the rocprim path to hipstdpar and created a default value for another unrelated flag.

I have also added the commits from this #506 (comment) MR.

@NB4444 NB4444 changed the base branch from develop to release-staging/rocm-rel-6.4 December 23, 2024 14:39
@stanleytsang-amd
Copy link
Collaborator

I'm encountering build errors when making the hipstdpar tests in Ubuntu (haven't tried other OSes but I assume they will be the same) when doing:

CXX=hipcc cmake -DBUILD_HIPSTDPAR_TEST=ON -DGPU_TARGETS=gfx942 ..
make -j

[ 20%] Building C object CMakeFiles/sqlite3.dir/_deps/sqlite_local-src/sqlite3.c.o
[ 40%] Building CXX object test/hipstdpar/CMakeFiles/interpose_compile.hip.dir/test_interpose.cpp.o
[ 60%] Building CXX object test/hipstdpar/CMakeFiles/algorithms_compile.hip.dir/test_algorithms.cpp.o
[ 60%] Built target sqlite3
[ 80%] Linking CXX executable interpose_compile.hip
[ 80%] Built target interpose_compile.hip
[100%] Linking CXX executable algorithms_compile.hip
ld.lld: error: undefined symbol: tbb::detail::r1::isolate_within_arena(tbb::detail::d1::delegate_base&, long)
>>> referenced by test_algorithms.cpp
>>>               CMakeFiles/algorithms_compile.hip.dir/test_algorithms.cpp.o:(main)
>>> referenced by test_algorithms.cpp
>>>               CMakeFiles/algorithms_compile.hip.dir/test_algorithms.cpp.o:(main)
>>> referenced by test_algorithms.cpp
>>>               CMakeFiles/algorithms_compile.hip.dir/test_algorithms.cpp.o:(std::result_of<__pstl::execution::v1::parallel_unsequenced_policy const& ()>::type __pstl::__internal::__except_handler<void __pstl::__internal::__pattern_inplace_merge<__pstl::execution::v1::parallel_unsequenced_policy const&, __gnu_cxx::__normal_iterator<int*, std::vector<int, std::allocator<int>>>, std::less<int>, std::integral_constant<bool, true>>(__pstl::execution::v1::parallel_unsequenced_policy const&, __gnu_cxx::__normal_iterator<int*, std::vector<int, std::allocator<int>>>, __gnu_cxx::__normal_iterator<int*, std::vector<int, std::allocator<int>>>, __gnu_cxx::__normal_iterator<int*, std::vector<int, std::allocator<int>>>, std::less<int>, std::integral_constant<bool, true>, std::integral_constant<bool, true>)::'lambda'()>(__pstl::execution::v1::parallel_unsequenced_policy const&))
>>> referenced 9 more times

ld.lld: error: undefined symbol: tbb::detail::r1::allocate_memory(unsigned long)

and so on. Even though we did not specify -DBUILD_HIPSTDPAR_TEST_WITH_TBB=ON, it looks like the hipstdpar tests still use TBB. Indeed, if manually add -ltbb to the build command, it builds properly.

According to https://stackoverflow.com/questions/67854770/i-do-not-use-tbb-but-i-get-linker-errors-related-to-tbb, it seems like when building with clang, it requires libtbb...

@NB4444
Copy link
Contributor Author

NB4444 commented Dec 24, 2024

It seems to be a weird dependency, because it seems to be only needed to add the flag when TBB is installed. I tested it on the rocm/dev-ubuntu-24-04:latest which does not have any installation of TBB, which worked fine. After I installed libttb-dev I got the same compile error. So I changed the code to always add the flag when a TBB installation is found, this should fix this issue. If we keep getting more of these issues, it might be better to just disable these tests as a default or use something like try_compile in cmake to not let this hinder the other rocthrust tests.

@stanleytsang-amd
Copy link
Collaborator

Build failures are due to compiler bugs which will be resolved in upcoming newer builds. Otherwise, it looks good to me.

@stanleytsang-amd stanleytsang-amd merged commit d5c9a92 into ROCm:release-staging/rocm-rel-6.4 Dec 24, 2024
5 of 9 checks passed
umfranzw added a commit that referenced this pull request Feb 26, 2025
* Enable HIP language (#493)

If the HIP language can be used, the HIP_USECXX variable sets rocThrust to use the CMake HIP language rather than CXX.

Co-authored-by: Lauren Wrubleski <[email protected]>

* Made TBB optional for hipstdpar tests (#507)

* added TBB in dependencies cmake

* updated changelog

* removed duplicate dependencies declaration in test cmake

* Create optional flag to run tests hipstdpar with TBB

* Add TBB optional flag to CHANGELOG

* Add rocprim location to hipstdpar

* Add default and test for DONWLOAD_ROCRAND

* Build with TBB when available or when FLAG is set

---------

Co-authored-by: NguyenNhuDi <[email protected]>

* Separate CMake BUILD_TEST and BUILD_HIPSTDPAR_TEST options (#508)

* Separate CMake BUILD_TEST and BUILD_HIPSTDPAR_TEST options

Previously, enabling BUILD_TEST would also enable hipstdpar tests
if we detected that a c++17-capable compiler was present. However,
this caused build issues on systems with a c++17 compiler but an
outdated version of libstdc++ that didn't support c++17 (RHEL 8.x).

Currently, we require a minimum cmake version of 3.10.2. There's
no real robust way of detecting the libstdc++ version that will work
that far back.

To workaround this problem for now, this change splits the BUILD_TEST
and BUILD_HIPSTDPAR_TEST cmake options so that they are independent.
This means that in order to enable hipstdpar tests, the user must
explicitly enable the BUILD_HIPSTDPAR_TEST option.

Update the readme to reflect this.

* Update README.md

Co-authored-by: Leo Paoletti <[email protected]>

---------

Co-authored-by: Di Nguyen <[email protected]>
Co-authored-by: Leo Paoletti <[email protected]>

* Updated known issue for inclusive_scan_by_key compiler bug (#513)

* updated the known issue

* Update CHANGELOG.md

Co-authored-by: spolifroni-amd <[email protected]>

* moved known issue to 6.4 section

* updated to include that the issue has been fixed

---------

Co-authored-by: spolifroni-amd <[email protected]>

* Extend fallback coverage for copy_if (#512)

We recently added a "fallback" implementation for thrust::copy_if that
is invoked when copying a custom type that's too large to fit in
shared memory.

This change extends the fallback slightly so that it can be used with
an overload of copy_if that accepts a stencil buffer (to copy by
key). It also adds a unit test to cover this case.

It also fixes a small bug in the fallback implementation that could
cause the scan accumulator type to overflow when the results are
compacted.

---------

Co-authored-by: Lauren Wrubleski <[email protected]>
Co-authored-by: Nick Breed <[email protected]>
Co-authored-by: NguyenNhuDi <[email protected]>
Co-authored-by: Di Nguyen <[email protected]>
Co-authored-by: Leo Paoletti <[email protected]>
Co-authored-by: spolifroni-amd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants