Multicolor (independent set) reordering capability on reference executor - #2006
Multicolor (independent set) reordering capability on reference executor#2006Slaedr wants to merge 11 commits into
Conversation
yhmtsai
left a comment
yhmtsai
left a comment
There was a problem hiding this comment.
you also need to modify test_install.cpp
|
@yhmtsai None of the other reorder classes are there in test_install.cpp. Should I still add a check for building a Multicolor object there? |
pratikvn
left a comment
pratikvn
left a comment
There was a problem hiding this comment.
some more comments. But mostly looks good. One general question from my side: Does this reorder setup suit your workflow ? Meaning can you make use of this in your Gauss-Seidel ?
| * The first entry is always 0, since the first color always starts at 0. | ||
| * The last entry stores the total number of rows. | ||
| */ | ||
| std::vector<index_type> get_color_pointers() const { return color_ptrs_; } |
There was a problem hiding this comment.
It might make sense to have this as a gko::array rather than a std::vector.
There was a problem hiding this comment.
This makes it explicit that the color_ptrs is always on the host. Do you expect a situation in which someone needs this on the device instead?
There was a problem hiding this comment.
@pratikvn I can change the std::vector to gko::array if you say that is preferable, even though I can't (currently) think of a use case where an application will want the color pointers on the device.
There was a problem hiding this comment.
I think keeping all internal arrays as gko::array objects is better than having some as std::vector. But if you need some methods of std::vector, then it should be fine.
There was a problem hiding this comment.
one more reason to prefer gko::array to std::vector is that the user can track allocations (through a logger), and switching allocators is easier. For example, using a PoolAllocator like Umpire, once support for that is added. If you need to call std::vector routines, maybe use gko::vector, which is constructible with an executor. See core/base/allocator.hpp:161
| // assert(permutation.end() == permutation.begin() + | ||
| // color_ptrs[num_colors]); |
There was a problem hiding this comment.
I will uncomment this part for checking?
There was a problem hiding this comment.
Looks like this was left over from where I initially implemented this a while back. Since permutation is a raw pointer here, it does not make sense. I'll just remove it.
| ASSERT_EQ(this->mc_factory->get_executor(), this->exec); | ||
| } | ||
|
|
||
| TYPED_TEST(Multicolor, GeneratesCorrectOrderingWithCsrInput) |
There was a problem hiding this comment.
I think the followings should be in the reference test?
There was a problem hiding this comment.
Since this testing a particular generate, I think it should be in core.
There was a problem hiding this comment.
I moved it to reference; I think you're right that this might make more sense.
|
@Slaedr , does the approach of having Multicolor as a reordering work for the goal of having this within GaussSeidel ? |
|
@pratikvn Yes. I'm not sure if it's best offered as an option within the Gauss-Seidel class(es), since the matrix needs to be modified to obey the ordering. The best workflow is probably to reorder the matrix and then generate GaussSeidel on the reordered matrix, passing in the color pointers. If the user does not supply the color pointers, I guess we could compute and store a reordered copy of the matrix in the generate step. |
|
@Slaedr , can you please check that you can use reordering as a wrapper for Gauss-Seidel ? If so, then I dont have any other issues with this PR. |
| const auto local_nrows = num_vertices; | ||
|
|
||
| std::vector<int> color(local_nrows, -1); |
There was a problem hiding this comment.
If we already know the size beforehand, then we should allocate the vector outside the kernel in core.
There was a problem hiding this comment.
Unfortunately, the number of helper vectors needed depends on the backend. The parallel backends need more than one working array (at least for now), though the reference backend needs only one. So, to keep the kernel interface the same, would you say it's best to leave it as it is, or is there a better solution?
| * The first entry is always 0, since the first color always starts at 0. | ||
| * The last entry stores the total number of rows. | ||
| */ | ||
| std::vector<index_type> get_color_pointers() const { return color_ptrs_; } |
There was a problem hiding this comment.
I think keeping all internal arrays as gko::array objects is better than having some as std::vector. But if you need some methods of std::vector, then it should be fine.
|
@Slaedr do you think anything to check from my side? Otherwise, this PR can be merged after the pipeline are passed. Do you still have access to ginkgo repo? We currently only process CI from ginkgo own repo. |
Looks like I don't have access. The workflows are waiting for approval. |
3 participants
The aim is to have a parallel GPU-capable multicolor ordering using the JPL algorithm. This PR only has the sequential reference implementation and corresponding tests.
Currently, CSR and SparsityCSR inputs are supported.
Also adds a "cores per SM" entry for NVIDIA GB10 GPU.