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

Thrust 1.10 Fixes #352

Merged
merged 4 commits into from
May 13, 2021
Merged

Conversation

pbauman
Copy link
Contributor

@pbauman pbauman commented May 3, 2021

This PR addresses Thrust 1.10 breaking changes. Thrust 1.10 landed in CUDA 11.2 and will land in ROCm 4.2.

As discussed in NVIDIA/thrust#1379 (that internally references NVIDIA/thrust#1176), the behavior of exclusive_scan and inclusive_scan changed in the case where the input types and output types were not the same. There is no deprecation warning or error thrown by the compiler. Indeed, with Thrust 1.10, before this PR, the exclusive_scan calls that had make_transform_iterator used in the input types (silently!) generated incorrect results. That means HYPRE is broken on GPUs today with CUDA 11.2 without this PR. There are a couple of way to fix, but what I did for the fix for exclusive_scan was to just use the API where one specifies the initial value and that was enough to fix the issue. It did not appear to me that there were any inclusive_scan calls affected in HYPRE. And my tests with this PR with a ROCm 4.2 release candidate pass.

In addition, Thrust 1.10 deprecated the use of C++ before C++14 so I've added -std=c++14 to the HIPCXXFLAGS argument in the configure.in (and bootstrapped).

Thrust 1.12 introduces similar breakages for the scan_by_key cousins, see NVIDIA/thrust#1376. The fixes are similar and I dropped in explicit casts to HYPRE_BigInt in the (already existing) initial value for exclusive_scan_by_key (commit 9a3bb66). I've not tried to address inclusive_scan_by_key cases, but I do believe they will be broken. I strongly recommend adding unit tests for those calls and adding them to the test suite. Thrust 1.12 is supposedly going to land in CUDA 11.4 (at least according to that thrust release page). I do not know when it will land in a ROCm release.

Thank you.

@pbauman pbauman requested a review from liruipeng May 3, 2021 13:04
@liruipeng
Copy link
Contributor

liruipeng commented May 3, 2021

@pbauman Thank you for fixing this issue, which would be very hard to find without knowing the behavior change of thrust. For future development, do you know where we should keep looking at for such kind of changes? We have been using https://thrust.github.io/doc/index.html for main reference of thrust, which has been apparently not up to date.

About C++14, we are aware of the fact C++11 has been deprecated by thrust >= 11.0. Currently we just ignore the warnings (

#if CUDA_VERSION >= 11000
). Probably we should change hypre's required C++ version to C++14. Any ideas on this?

Thanks!

@pbauman
Copy link
Contributor Author

pbauman commented May 4, 2021

For future development, do you know where we should keep looking at for such kind of changes?

Honestly, I don't have a good answer. It took me some time to narrow down the issue and then some additional digging by me and a member of our libraries team to ultimately find the GitHub issue I linked. In particular, the first change wasn't even documented in the release notes of Thrust, so there was no way to know without essentially watching every commit to Thrust. It's very frustrating and unnerving, to say the least. Unfortunately, I don't have any good ideas.

Probably we should change hypre's required C++ version to C++14. Any ideas on this?

Ultimately, that's up to the HYPRE developers. But, as far as from the ROCm/HIP side, the compiler supports C++17 I believe, so requiring C++14 there is not a problem; furthermore, ROCm 4.1 is the minimum version that's really supported here in HYPRE because there was a compiler bug prior to that release that generated incorrect code in one of the kernels (when using -O2 or higher). As far as I understand, C++ only infiltrates HYPRE in the GPU code paths, so I guess the question to ask would be how far back in time do you want to support CUDA and then, once that's determined, what C++ standard is supported by that compiler? Just my two cents.

@liruipeng liruipeng changed the base branch from master to cuda11.2 May 13, 2021 05:58
@liruipeng liruipeng merged commit 827c684 into hypre-space:cuda11.2 May 13, 2021
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.

2 participants