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

Develop stream 2025-01-20 #455

Merged
merged 24 commits into from
Feb 28, 2025

Conversation

NB4444
Copy link
Contributor

@NB4444 NB4444 commented Jan 20, 2025

Develop stream 2025-01-20 changes, needs rocprim stream updates:

Adopt symbol visibility rules in hipCUB

CCCL/thrust 2.6.0 compatibility changes

  • General CCCL/thrust 2.6.0 changes
  • Add UnrolledThreadLoad and UnrolledThreadCopy functions

Documentation

  • Fix warnings when generating sphinx docs
  • Remove extra dashes in the documentation its \param and \tparam
  • Update documentation generation instructions

General changes and bug fixes

  • Add hipGraph capture tests for device run length encode
  • Test output locations for large indices
  • fix GenerateResourceSpec for gfx90a
  • style: add include-what-you-use pragmas to hipcub
  • log more information during configure
  • Make HIPCUB_HOST API also accessible on device in single_pass_scan_operators.hpp
  • Fix compile error in hipCUB with lookback_scan_prefix_flag name change in rocprim
  • Update thread_store and thread_load for parity.
  • Some internal changes to the CI

@NB4444 NB4444 changed the title Develop upstream 2025 01 20 Develop stream 2025 01 20 Jan 20, 2025
@NB4444 NB4444 changed the title Develop stream 2025 01 20 Develop stream 2025-01-20 Jan 20, 2025
@NB4444 NB4444 marked this pull request as ready for review January 21, 2025 12:35
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.

The changelog needs some clarification and changes. I've made some suggestions.

CHANGELOG.md Outdated

### Known issues

* The deprecated `BlockAdjacentDifference::FlagHeads`, `BlockAdjacentDifference::FlagTails` and `BlockAdjacentDifference::FlagHeadsAndTails` are not present in CCCL (CUB) from release 2.6.0 on, and thus neither they are in hipCUB's CUB backend for versions strictly greater than 3.4.0. These will be definitely removed from both backends in a future release.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be under "Removed" rather than Known Issues

Suggested change
* The deprecated `BlockAdjacentDifference::FlagHeads`, `BlockAdjacentDifference::FlagTails` and `BlockAdjacentDifference::FlagHeadsAndTails` are not present in CCCL (CUB) from release 2.6.0 on, and thus neither they are in hipCUB's CUB backend for versions strictly greater than 3.4.0. These will be definitely removed from both backends in a future release.
*`BlockAdjacentDifference::FlagHeads`, `BlockAdjacentDifference::FlagTails`, and `BlockAdjacentDifference::FlagHeadsAndTails` have been deprecated. They were removed in CCCL (CUB) 2.6.0 and have been removed in all versions of hipCUB's CUB backend later than 3.4.0.

When you say "strictly greater than 3.4.0" do you mean from 3.4.1 and on, or do you mean something like 3.4.0.1 and on?

Also I'm confused by this line: "These will be definitely removed from both backends in a future release."

Have they been removed or not? Or do you mean they'll be removed from a different backend in a future release? If they will be removed from a different backend in a future release, that info should be added to the "Upcoming Changes" section.

Copy link
Contributor

@Beanavil Beanavil Jan 23, 2025

Choose a reason for hiding this comment

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

When you say "strictly greater than 3.4.0" do you mean from 3.4.1 and on, or do you mean something like 3.4.0.1 and on?

AFAIK we only do mayor.minor.patch kind of releases, hence it's meant to be from 3.4.1 on. However, even if a version such as 3.4.0.1 is possible, the update to CCCL 2.6.0 has been done after the 3.4.0 release of hipCUB, so regardless of the next version number the dependency on CCCL 2.6.0 will be there and hence the absence of these functions in the CUB backend of hipCUB.

Also I'm confused by this line: "These will be definitely removed from both backends in a future release."

Have they been removed or not? Or do you mean they'll be removed from a different backend in a future release?

They have been removed from CCCL/CUB (and thus from the CUB backend of hipCUB) as mentioned. However, as discussed internally, these should not be removed from the rocPRIM backend until the next ROCm mayor release so what the sentence means is that these will be dropped from the rocPRIM backend as well in some future release when the next ROCm mayor version is released.

If they will be removed from a different backend in a future release, that info should be added to the "Upcoming Changes" section.

I see, we'll do this!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and also, regarding the suggested changes:

*BlockAdjacentDifference::FlagHeads, BlockAdjacentDifference::FlagTails, and BlockAdjacentDifference::FlagHeadsAndTails have been deprecated.

These were already deprecated, so I think the original sentence should be ok. Perhaps something like:

* The already-deprecated `BlockAdjacentDifference::FlagHeads`, `BlockAdjacentDifference::FlagTails` and `BlockAdjacentDifference::FlagHeadsAndTails` ...

would make it more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

In which version were they deprecated? That could make it clearer. Something like "[functions] that were deprecated in [version] have now been removed"

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this sound (with the right version put in instead of VERSION_X obvs :) ):

* `BlockAdjacentDifference::FlagHeads`, `BlockAdjacentDifference::FlagTails` and `BlockAdjacentDifference::FlagHeadsAndTails` have been removed from hipCUB's CUB backend as of version 3.4.1. They were already removed from CCCL (CUB) as of version 2.6.0 and deprecated in VERSION_X of hipCUB's CUB backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good! My final suggestion after some re-writting to clarify which version is hipCUB's and which is CCCL's:

### Known issues

* `BlockAdjacentDifference::FlagHeads`, `BlockAdjacentDifference::FlagTails` and `BlockAdjacentDifference::FlagHeadsAndTails` have been removed from hipCUB's CUB backend. They were already deprecated as of version 2.12.0 of hipCUB and they were removed from CCCL (CUB) as of CCCL's 2.6.0 release.
...

### Upcoming Changes

* `BlockAdjacentDifference::FlagHeads`, `BlockAdjacentDifference::FlagTails` and `BlockAdjacentDifference::FlagHeadsAndTails`, already deprecated as of version 2.12.0 of hipCUB, will be removed from the rocPRIM backend in a future release for the next ROCm major version (ROCm 7.0.0).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd smooth it a bit, but otherwise I'm good with this! Clarity is very appreciated! :)

BlockAdjacentDifference::FlagHeads, BlockAdjacentDifference::FlagTails and BlockAdjacentDifference::FlagHeadsAndTails were deprecated as of version 2.12.0 of hipCUB, and will be removed from the rocPRIM backend in a future release for the next ROCm major version (ROCm 7.0.0).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm gonna approve so you don't have to wait for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

done!

@NB4444 NB4444 force-pushed the develop_upstream_2025_01_20 branch from c77a7b4 to 1a7cf6c Compare January 23, 2025 09:10
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.

One last thing :)

@Beanavil Beanavil force-pushed the develop_upstream_2025_01_20 branch from 5d84fef to d7b76ca Compare January 28, 2025 11:03
@Beanavil Beanavil force-pushed the develop_upstream_2025_01_20 branch from d7b76ca to f2071b1 Compare January 29, 2025 11:59
@NB4444 NB4444 force-pushed the develop_upstream_2025_01_20 branch from 67c4136 to 986a1fb Compare February 21, 2025 07:12
@NB4444
Copy link
Contributor Author

NB4444 commented Feb 21, 2025

Resolved some merge conflicts.

Copy link
Collaborator

@umfranzw umfranzw left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@stanleytsang-amd stanleytsang-amd merged commit d0c39fc into ROCm:develop Feb 28, 2025
6 of 13 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.