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

Add syclSolverInverter #4118

Merged
merged 5 commits into from
Jul 19, 2022
Merged

Conversation

jngkim
Copy link
Contributor

@jngkim jngkim commented Jul 15, 2022

Please review the developer documentation
on the wiki of this project that contains help and requirements.

Proposed changes

This PR contains the changes provided by Ye's review #9(ye-luo#9) and #10.

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

Added src/QMCWaveFunctions/Fermion/syclSolverInverter.hpp
Replaced abort with proper error handling method
Removed a dead code

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

  • Intel PVC
  • oneAPI 2022.2

Checklist

  • Yes. This PR is up to date with current the current state of 'develop'
  • Yes. Code added or changed in the PR has been clang-formatted
  • No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
  • No. Documentation has been added (if appropriate)

@ye-luo ye-luo self-requested a review July 15, 2022 14:48
@ye-luo ye-luo changed the title Sycl allocator merge 2207 with revision Add syclSolverInverter Jul 15, 2022
Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

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

LGTM. Still non-ANL approval

@prckent
Copy link
Contributor

prckent commented Jul 15, 2022

Thank you.

If I have oneAPI 2022.2 and no GPU, will this run and what is the cmake invocation to do so?

@ye-luo
Copy link
Contributor

ye-luo commented Jul 15, 2022

Thank you.

If I have oneAPI 2022.2 and no GPU, will this run and what is the cmake invocation to do so?

ENABLE_SYCL=ON. The added code doesn't require OpenMP offload.
SYCL techinically works without GPU but that requires proper device selection and MKL support. So likely you cannot run without an Intel GPU.

Copy link
Contributor

@PDoakORNL PDoakORNL left a comment

Choose a reason for hiding this comment

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

Small question on CMake and testing should not have unecessary #ifdef QMC_COMPLEX. Beyond PR scope need to resolve QMCPACK naming convention vs. solverwrapper classes.

* @tparam T_FP high precision for matrix inversion, T_FP >= T
*/
template<typename T_FP>
class syclSolverInverter
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice if this followed the naming conventions for classes. Although an unfortunate precedent has be set by other solver wrapper classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it aligned as other programming models. rocSolver, cuSolver.

@@ -873,7 +873,7 @@ if(ENABLE_SYCL)
endif()
add_library(SYCL::host INTERFACE IMPORTED)
add_library(SYCL::device INTERFACE IMPORTED)
find_package(IntelDPCPP REQUIRED CONFIGS IntelDPCPPConfig-modified.cmake PATHS ${PROJECT_CMAKE})
find_package(IntelDPCPP REQUIRED CONFIGS IntelDPCPPConfig-modified.cmake PATHS ${PROJECT_CMAKE} NO_DEFAULT_PATH)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why NO_DEFAULT_PATH here. I think this just prevents finding the package in the location CMake does/will eventually think is standard? Can't this still be overridden with -DIntelDPCPP_ROOT=...

Copy link
Contributor

Choose a reason for hiding this comment

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

IntelDPCPPConfig-modified.cmake is my in house modified version. So it should not be looked up outside qmcpack root.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I read this too fast, makes sense.

Copy link
Contributor

@PDoakORNL PDoakORNL left a comment

Choose a reason for hiding this comment

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

Since my remaining concerns apply equally to the other solver wrappers it makes sense to address them in other PRs. So this LGTM

@prckent
Copy link
Contributor

prckent commented Jul 18, 2022

Test this please

@prckent prckent enabled auto-merge July 18, 2022 23:29
@prckent prckent merged commit 8e5221c into QMCPACK:develop Jul 19, 2022
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