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

Update batched matrix inversion and update frontier recipe #4742

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

ye-luo
Copy link
Contributor

@ye-luo ye-luo commented Sep 20, 2023

Proposed changes

Resolves #4741 by deleting the non-pivoting code path that needs kernels.
The non-pivoting case was needed likely by the legacy CUDA which was deleted.

What type(s) of changes does this code introduce?

  • Refactoring (no functional changes, no api changes)

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

epyc-server

Checklist

  • Yes. This PR is up to date with current the current state of 'develop'

Restore hipBLAS.hip.cpp to hipBLAS.cpp as a pure CXX source file with HIP kernels removed.
@ye-luo ye-luo changed the title Update batched matrix inv Update batched matrix inversion and update frontier recipe Sep 20, 2023
@ye-luo
Copy link
Contributor Author

ye-luo commented Sep 20, 2023

@jakurzak I was reading #3341 (comment). What is the current behavior of hipblas?getrfBatched() and hipblas?getriBatched()? Are they unconditionally hitting the non-pivoting case or they behave like cuBLAS selecting code path depending on whether the pointer is nullptr? In the former case, the current code is good. In the latter case, we may further simplify the code in QMCPACK.

@ye-luo
Copy link
Contributor Author

ye-luo commented Sep 20, 2023

Test this please

Copy link
Contributor

@prckent prckent left a comment

Choose a reason for hiding this comment

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

The problem with asserts is that they are only active in debug builds. We don't have any ROCm debug builds in testing. Throw an exception instead?

@ye-luo
Copy link
Contributor Author

ye-luo commented Sep 20, 2023

The problem with asserts is that they are only active in debug builds. We don't have any ROCm debug builds in testing. Throw an exception instead?

Agreed. It is pure C++ now and thus exception is better. I was concerned of C++ and HIP interaction.

@ye-luo
Copy link
Contributor Author

ye-luo commented Sep 20, 2023

Test this please.

Copy link
Contributor

@prckent prckent left a comment

Choose a reason for hiding this comment

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

Simpler.

rtd build for nexus docs is sick but this PR does not touch any docs. Hopefully a transient problem.

@prckent prckent merged commit c422bdf into QMCPACK:develop Sep 20, 2023
21 checks passed
@ye-luo ye-luo deleted the update-batched-matrix-inv branch September 20, 2023 22:22
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.

Spin off identity kernels needed
2 participants