Development of hiopVectorCuda
#572
Conversation
nychiang
force-pushed
the
cuda-vector-dev
branch
from
b3ca53d
to
6384674
Compare
nychiang
force-pushed
the
cuda-vector-dev
branch
from
a57c440
to
47b10b5
Compare
| set_match_pattern_cu<<<num_blocks,block_size>>>(n_local, yd, xd, id); | ||
| } | ||
|
|
||
| /** @brief Adjusts duals. */ |
There was a problem hiding this comment.
I only skimmed this file, but I assume these comments should be in the header instead?
There was a problem hiding this comment.
I think it i helpful to have brief description of each function next to the source code and the long one in the header files. Doxygen can merge them.
There was a problem hiding this comment.
There are minor issues that @cameronrutherford and I pointed out that can be easily addressed. As soon as these are done, this PR can be merged.
In a more general context, I have mixed feelings about this PR. It essentially duplicates the capability that already exists in RAJA modules and therefore eliminates RAJA/Umpire as HiOp dependencies. On the other hand, it adds a couple of thousands lines of code more, only for CUDA capability. You would have to replicate the same for HIP and oneAPI, only to replace the RAJA dependency. I don't think this path is sustainable, but this is just my opinion. In any case, we have a real-life experiment to asses the value of portability libraries :).
| set_match_pattern_cu<<<num_blocks,block_size>>>(n_local, yd, xd, id); | ||
| } | ||
|
|
||
| /** @brief Adjusts duals. */ |
There was a problem hiding this comment.
I think it i helpful to have brief description of each function next to the source code and the long one in the header files. Doxygen can merge them.
| * @file hiopVectorIntCuda.cpp | ||
| * |
There was a problem hiding this comment.
This file should probably have extension *.cu, to emphasize it can be compiled with CUDA only.
implementation of "hiopVectorCuda"
Maybe this file/class should be named as "hiopVectorCudaPar" since it also supports MPI (verified by unit tests).
Right now I manually set "mem_space" to "cuda" to run the unit tests. More works are required to address this issue after we merge #543 (memory backends and execution policies)