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

Mergeback 6.4 Fixes #523

Merged
merged 6 commits into from
Feb 26, 2025
Merged

Mergeback 6.4 Fixes #523

merged 6 commits into from
Feb 26, 2025

Conversation

umfranzw
Copy link
Collaborator

Merge changes made in the 6.4 release-staging / release branches back into develop.

umfranzw and others added 6 commits December 3, 2024 16:19
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]>
* 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

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 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]>
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.
This change merges release/rocm-rel-6.4 back into develop.
Copy link
Contributor

@spolifroni-amd spolifroni-amd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question about BUILD_HIPSTDPAR_TEST_WITH_TBB and the readme.

@@ -13,6 +13,8 @@ Documentation for rocThrust available at

### Added

* Added a section to install Thread Building Block (TBB) inside `cmake/Dependencies.cmake` if TBB is not already available.
* Made Thread Building Block (TBB) an optional dependency with the new `BUILD_HIPSTDPAR_TEST_WITH_TBB` flag, default is `OFF`. When the flag is `OFF` and TBB is not already on the machine it will compile without TBB. Otherwise is will compile it with TBB.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this added to the readme? I'm not seeing it in there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - I don't see any entry in the readme, either in the develop or release branches, so it probably was never added.
Would it be ok to add it in a follow-up PR, since this one is intended to merge back existing fixes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats fine.

@umfranzw umfranzw changed the title Mergeback 6.4 Mergeback 6.4 Fixes Feb 24, 2025
Copy link
Contributor

@NguyenNhuDi NguyenNhuDi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@umfranzw umfranzw merged commit 0006584 into ROCm:develop Feb 26, 2025
13 of 20 checks passed
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.

5 participants