GPU support SparseTable<INFO> for assembly #929
Conversation
multitalentloes
added
the
manual:irrelevant
opm/grid/utility/SparseTable.hpp
Outdated
| { | ||
| std::vector<TemplatedStruct<ResidualNBInfoType, MiniMatrixType>> minimatrices(cpu_matrix.dataSize()); | ||
| for (auto e : cpu_matrix.dataStorage()) { | ||
| minimatrices.push_back(e); // will this be converted correctly??? |
There was a problem hiding this comment.
You will end up with minimatrices having twice the size it should, either give it size on construction or use push_back, not both.
There was a problem hiding this comment.
This is a completely criminal error on my side...
opm/grid/utility/SparseTable.hpp
Outdated
| ); | ||
| } | ||
|
|
||
| // Since we cannot have MatrixBlock in GPU code, we need to convert to MiniMatrix |
There was a problem hiding this comment.
The whole existence of this specialization is a bit of a hack... I suggest that we place it with the definition of the struct (where it can be concrete and need not be templatized) instead, it does not have to be placed here.
There was a problem hiding this comment.
Not really sure how to avoid this. I suppose we want to end up with this being a very straight-forward initialization where we can just use SparseTable(GPUBuf1, GPUBuf2) and have the conversion of the stuff in the struct be handled automatically. I have made a couple of attempts at this but I have not gotten anything to work smoothly...
multitalentloes
force-pushed
the
start_porting_assembly_to_gpu
branch
from
8964fad to
6eb430b
Compare
|
Jenkins build this serial rocm hipify opm-simulators=4686 please |
|
Jenkins build this opm-simulators=4686 serial rocm hipify please |
|
jenkins build this opm-simulators=4686 serial rocm hipify please |
I don't see how OPM/opm-simulators#4686 is related to this. Did you mean something else instead? |
|
You are right, I was supposed to start jenkins with #6508 from OPM simulators |
|
jenkins build this opm-simulators=6508 serial rocm hipify please |
1 similar comment
|
jenkins build this opm-simulators=6508 serial rocm hipify please |
opm/grid/utility/SparseTable.hpp
Outdated
| #include <opm/simulators/linalg/gpuistl/GpuBuffer.hpp> | ||
| #include <opm/simulators/linalg/gpuistl/GpuView.hpp> | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
why not just gpuistl_if_available?
opm/grid/utility/SparseTable.hpp
Outdated
| { | ||
| // removed for now because we cannot access the zero'th element if Storage is a GpuBuffer | ||
| // OPM_ERROR_IF(row_start_.size() == 0 || row_start_[0] != 0, | ||
| // "Invalid row_start array"); |
There was a problem hiding this comment.
I think storage can probably be passed as an rvalue reference:
Storage<T>&& data, Storage<int>&& row_starts it will save us an allocation and a copy on the gpu.
There was a problem hiding this comment.
also, I would either remove the commented out code, or remove the comment and have the exception in an if constexpr (std::is_same_v<Storage<int>, std::vector<int>>)
There was a problem hiding this comment.
good suggestion, done
opm/grid/utility/SparseTable.hpp
Outdated
| // Since we cannot have MatrixBlock in GPU code, we need to convert to MiniMatrix | ||
| // For now I think this is an okay place to convert types | ||
| template<class MatrixBlockType, class MiniMatrixType, class ResidualNBInfoType, template <typename, typename...> class TemplatedStruct> | ||
| auto copy_to_gpu(const SparseTable<TemplatedStruct<ResidualNBInfoType, MatrixBlockType>>& cpu_matrix) |
There was a problem hiding this comment.
are you sure this is the correct template for sparsetable? TemplatedStruct looks like a Storage (should probably be renamed for clarity), but you are inserting it into the SparseTable with the templates, while SparseTable takes two template arguments, one of them the type T and then `Storage. I think a simpler signature would be something like
template<class MatrixBlockType, class MiniMatrixType>
auto copy_to_gpu(const SparseTable<MatrixBlockType, std::vector>& cpu_matrix)There was a problem hiding this comment.
Ok, after having this explained I get it. I would however recommend these functions being moved to opm-simulators and having the struct names spelled out rather than having it as a template argument. First of all it will make the code a lot clearer, but secondly it will remove the potential of a wrong overload.
|
jenkins build this opm-simulators=6508 serial rocm hipify please |
4 similar comments
|
jenkins build this opm-simulators=6508 serial rocm hipify please |
|
jenkins build this opm-simulators=6508 serial rocm hipify please |
|
jenkins build this opm-simulators=6508 serial rocm hipify please |
|
jenkins build this opm-simulators=6508 serial rocm hipify please |
opm/grid/utility/SparseTable.hpp
Outdated
| inline constexpr bool always_false_v = false; | ||
|
|
||
| // Poison iterator is a helper class that will allow for compilation but will fail at run-time | ||
| // Its intetion is to be used so that we can have a SparseTable of GPU data, which requires the |
There was a problem hiding this comment.
Since the class uses static_assert, it seems to me it will fail at compilation, not only at run-time?
Typo: intetion->intention
opm/grid/utility/SparseTable.hpp
Outdated
| } | ||
|
|
||
| SparseTable (Storage<T>&& data, Storage<int>&& row_starts) | ||
| : data_(data) |
There was a problem hiding this comment.
This must be
: data_(std::move(data))
to actually activate move semantics. Anything with a name must be moved from. Also row_starts.
multitalentloes
force-pushed
the
start_porting_assembly_to_gpu
branch
from
43ed086 to
c4f5acd
Compare
|
jenkins build this opm-simulators=6508 serial rocm hipify please |
4 participants
No description provided.