using reductions instead of handcrafted resolutions #466
Conversation
DeepCode failed to analyze this pull requestSomething went wrong despite trying multiple times, sorry about that. |
|
So clang is problematic. Hmmm... |
| @@ -10,7 +10,11 @@ list(REMOVE_ITEM VOTCA_SOURCES ${NOT_VOTCA_SOURCES}) | |||
| add_library(votca_xtp ${VOTCA_SOURCES}) | |||
| set_target_properties(votca_xtp PROPERTIES SOVERSION ${SOVERSION}) | |||
| add_dependencies(votca_xtp gitversion-xtp) | |||
| target_link_libraries(votca_xtp PUBLIC VOTCA::votca_csg VOTCA::votca_tools Boost::boost Eigen3::Eigen ${HDF5_LIBRARIES} PRIVATE LIBXC::LIBXC Boost::program_options Boost::filesystem Boost::system Boost::timer ) | |||
| target_link_libraries(votca_xtp PUBLIC VOTCA::votca_csg VOTCA::votca_tools Boost::boost Eigen3::Eigen ${HDF5_LIBRARIES} PRIVATE LIBXC::LIBXC Boost::program_options Boost::filesystem Boost::system Boost::timer ) | |||
| @@ -45,6 +45,13 @@ class Mat_p_Energy { | |||
| Mat_p_Energy(double e, Eigen::MatrixXd&& mat) | |||
| : _energy(e), _matrix(std::move(mat)){}; | |||
|
|
|||
| Mat_p_Energy operator+(const Mat_p_Energy& other) const { | |||
| @@ -51,8 +51,6 @@ Eigen::MatrixXd ReadMatrixFromString(const std::string& matrix) { | |||
|
|
|||
| BOOST_AUTO_TEST_CASE(small_basis) { | |||
|
|
|||
| OPENMP::setMaxThreads(1); | |||
There was a problem hiding this comment.
Why is it now possible to use multiple threads here?
There was a problem hiding this comment.
I am not sure why it was disabled before.
|
|
||
| const Eigen::VectorXd& fourc_vector = _fourcenter.get_4c_vector(); | ||
|
|
||
| Index dftBasisSize = DMAT.rows(); | ||
| Index vectorSize = (dftBasisSize * (dftBasisSize + 1)) / 2; | ||
| #pragma omp parallel for | ||
| #pragma omp parallel for reduction(+ : EXX) |
There was a problem hiding this comment.
The readability has improved significantly!
| #pragma omp parallel for schedule(guided) | ||
| Eigen::MatrixXd result = Eigen::MatrixXd::Zero(dst.rows(), dst.cols()); | ||
|
|
||
| #pragma omp declare reduction (+: Eigen::MatrixXd: omp_out=omp_out+omp_in)\ |
There was a problem hiding this comment.
I don't fully understand this snippet, could you please tell me how it works?
There was a problem hiding this comment.
omp_out=omp_out+omp_in is the communicative operation to perform and omp_priv is the "zero" element, right?
There was a problem hiding this comment.
if the reduction for MatrixXd has been declared in eigen.h why don't you use it here?
There was a problem hiding this comment.
this is in the eigen::internal namespace, so I had to do it again.
| target_link_libraries(votca_xtp PUBLIC VOTCA::votca_csg VOTCA::votca_tools Boost::boost Eigen3::Eigen ${HDF5_LIBRARIES} PRIVATE LIBXC::LIBXC Boost::program_options Boost::filesystem Boost::system Boost::timer ) | ||
|
|
||
| if(TARGET OpenMP::OpenMP_CXX) | ||
| target_link_libraries(votca_xtp PUBLIC OpenMP::OpenMP_CXX) |
There was a problem hiding this comment.
We are planning on any consumers for xtp if so we might need to add this to CMake exported target as well.
There was a problem hiding this comment.
I do not think there is a library which uses xtp but yeah.
Codecov Report
@@ Coverage Diff @@
## master #466 +/- ##
========================================
- Coverage 63.3% 63.3% -0.1%
========================================
Files 279 279
Lines 22795 22784 -11
========================================
- Hits 14434 14423 -11
Misses 8361 8361
Continue to review full report at Codecov.
|
Openmp is now required in version 4.5
this is not a big problem for gcc and icc but clang is a bit stupid, so you have to have clang-10 to use it properly. This seems to be a bug in the clang package.