Multiply sparse matrices #132
Conversation
…into multiply_sparse_matrices
…ltiply_sparse_matrices
…ltiply_sparse_matrices
…ltiply_sparse_matrices
|
Lets follow this PR up with a DistGraphv \cdot DistMatrix -> DistMatrix variant, which will obviate a DistSparse \cdot DistMatrix -> DistMatrix variant. |
| @@ -11,6 +11,206 @@ | |||
| #ifndef EL_CORE_DISTGRAPH_HPP | |||
| #define EL_CORE_DISTGRAPH_HPP | |||
|
|
|||
| #include <El/core/DistGraph/decl.hpp> | |||
There was a problem hiding this comment.
Is there a reason for purposefully breaking the convention of splitting the declarations and implementations into decl.hpp and impl.hpp files?
There was a problem hiding this comment.
It's been a very long time since I initially did these changes. However, this convention is a bit odd. It is not as if the impl.hpp is a .cpp file.
I propose that we (as much as possible) move to single header-only implementations.
Simply moving the decl/impl into one header is asking the pre-processor to do less work.
There was a problem hiding this comment.
In practice there is a lot of movement between the impl.hpp and an associated source file. For DistMatrix, there have been at least five such movements. If nothing else, consistency is important.
There was a problem hiding this comment.
@rhl- In general, you're asking to spend way more time compiling if you combine them. It is very common for header dependencies and transitive dependencies (header or source) to need only the declarations, which means that all those compilation units don't need to pull in the implementation and don't need to be recompiled any time the impl.hpp changes (usually much more common than decl.hpp changes). We saw a tangible improvement in PETSc compile times (especially incremental) when we split some headers.
There was a problem hiding this comment.
Hi Jed,
Not sure this is accurate. This is a pure header only implementation, with
separate sources, so you have to spend more time because its not all in one
place. However, I agree that there is value for only having signatures. If
impl.hpp was impl.cpp, then what you are saying is accurate.
Right now the headers extern template instantiations for a zillion things,
this is assuredly makes the builds very slow, because template
instantiation is relatively slow.
I'm trying to back out these changes for now.
Best,
-rhl
On Mon, Aug 22, 2016 at 4:19 PM Jed Brown notifications@github.com wrote:
In include/El/core/DistGraph.hpp
#132 (comment):@@ -11,6 +11,206 @@
#ifndef EL_CORE_DISTGRAPH_HPP
#define EL_CORE_DISTGRAPH_HPP-#include <El/core/DistGraph/decl.hpp>
@rhl- https://github.com/rhl- In general, you're asking to spend way
more time compiling if you combine them. It is very common for header
dependencies and transitive dependencies (header or source) to need only
the declarations, which means that all those compilation units don't need
to pull in the implementation and don't need to be recompiled any time the
impl.hpp changes (usually much more common than decl.hpp changes). We saw a
tangible improvement in PETSc compile times (especially incremental) when
we split some headers.—
You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub
https://github.com/elemental/Elemental/pull/132/files/32571ce8e750f142519b75f791742417c39b4bf9#r75777403,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATdUWJiGKjYBPYjywtwnQin6eGRvX_Oks5qii5lgaJpZM4HVOQ_
.
There was a problem hiding this comment.
Can you quantify that extra time to pull in both parts from separate files versus a unified file?
Also, I really am referring to splitting headers into the part that only declares the types and the part that contains the API associated with that type. It is common for dependent headers to only need the declaration -- only compilation units that actually manipulate that type need to include the rest of the header. You'll find many such examples in /usr/include/**/*{fwd,decl,type,types}.h{,pp}. Boost is insistent about factoring headers in that way, for example.
| try | ||
| { | ||
| Int m = 1; | ||
| for(int e = 1; e < 4; ++e){ |
There was a problem hiding this comment.
Consistent for loop style, please? The rest of the library would use:
for( Int e=1; e<4; ++e )
{
| @@ -0,0 +1,53 @@ | |||
| /* | |||
There was a problem hiding this comment.
I am only covering the routines I added for now. I have not yet added any Distributed multiply's. WHen I add then they will get tests, and i'll try at that point to cover more routines.
rhl-
force-pushed
the
multiply_sparse_matrices
branch
from
02f83ab
to
0a15f61
Compare
| @@ -235,7 +235,7 @@ class DistMatrix; | |||
| #include <El/core/Matrix/decl.hpp> | |||
| #include <El/core/Graph/decl.hpp> | |||
| #include <El/core/DistMap/decl.hpp> | |||
| #include <El/core/DistGraph/decl.hpp> | |||
| #include <El/core/DistGraph.hpp> | |||
There was a problem hiding this comment.
Why include DistGraph.hpp rather than DistGraph/decl.hpp?
* Refactor main CMakeLists * Update the CMake export for Hydrogen * Handle CUDA's extended lambda flag better * Add version number to shared libs.
* LLNL-hydrogen: (711 commits) Update README.md Add Clang 4.0 to .travis.yml and allow 6.0 to fail Fix ambiguous template special. errors with GCC 7+ Add GCC 8 and Clang 6.0 to Travis CI. Enable OpenMP code only if EL_HYBRID is set CMake, GCC: Enable libquadmath detection for GCC 5.x to 8.x. Pass MPI_C_COMPILER instead of CMAKE_C_COMPILER to ensure that ParMetis is built with the right compiler (flags) Cleanup some issues with ETI and the Memory class (elemental#136) Distributed Cholesky for GPU (elemental#135) Debugging TranslateBetweenGrids (elemental#133) Fix an issue leading to bad linkage. (elemental#134) Update CMake support for CUDA HPC SDK and HIP (elemental#132) Update HydrogenConfig.cmake.in Update CMakeLists.txt Update CMakeLists.txt Add assertion that LAPACK is found. (elemental#131) Serial eigensolver for GPU (elemental#130) Remove FP16 from the ETI for eigensolvers. (elemental#129) Changed the CUDA compiler detection to just enable CUDA support rather (elemental#128) Reenable the HermitianEig API for sequential matrices. (elemental#127) ...
Need to add unit tests.