Add parsec_advise_data_on_device for zpotrf_L #118
Conversation
| @@ -18,7 +18,8 @@ int main(int argc, char ** argv) | |||
| { | |||
| parsec_context_t* parsec; | |||
| int iparam[IPARAM_SIZEOF]; | |||
| dplasma_enum_t uplo = dplasmaUpper; | |||
There was a problem hiding this comment.
Don't forget to undo that at the end when the U is also done
There was a problem hiding this comment.
Why do you map the A matrix on lower ? You don't need to alter this variable, instead just call the function with dplasmaLower and add a comment on why you choose to do so.
QingleiCao
force-pushed
the
qinglei/potrf_gpu_advice
branch
from
eeb82ec
to
811e173
Compare
QingleiCao
force-pushed
the
qinglei/potrf_gpu_advice
branch
2 times, most recently
from
a4b81a9
to
f676e32
Compare
|
I uploaded the performance results. BTW, I updated to the latest PaRSEC. |
QingleiCao
force-pushed
the
qinglei/potrf_gpu_advice
branch
from
f676e32
to
a597442
Compare
|
I understand the performance benefits but I have many concerns about the approach. We (or at least I) claimed for a long time that JDF algorithms were expressing the dataflow and were independent of other platform related constraints. This PR is clearly at odds with this statement, and I'm not sure we want to go that route. |
|
As discussed, these changes will be moved to the wrapper to keep the JDF clean. |
QingleiCao
force-pushed
the
qinglei/potrf_gpu_advice
branch
2 times, most recently
from
c67b4b5
to
6743a5a
Compare
|
How about adding this feature in parsec instead of in dplasma? |
|
Can you give us a hint on how that would look in parsec ? |
I'm thinking of providing this feature in PaRSEC like |
QingleiCao
force-pushed
the
qinglei/potrf_gpu_advice
branch
2 times, most recently
from
9bfc9fc
to
78f8740
Compare
QingleiCao
force-pushed
the
qinglei/potrf_gpu_advice
branch
3 times, most recently
from
e0a77ea
to
252d978
Compare
QingleiCao
force-pushed
the
qinglei/potrf_gpu_advice
branch
from
252d978
to
726d4ab
Compare
| #if defined(DPLASMA_HAVE_CUDA) || defined(DPLASMA_HAVE_HIP) | ||
|
|
||
| /* Find all devices */ | ||
| void dplasma_find_nb_devices(int **dev_index, int *nb) { |
There was a problem hiding this comment.
This function has a generic name suggesting it gets all devices when internally it only selects the accelerators. Please change the name, and please add also support for ZE.
| if((*nb) == 0) { | ||
| char hostname[256]; | ||
| gethostname(hostname, 256); | ||
| fprintf(stderr, "No CUDA device found on rank %d on %s\n", |
There was a problem hiding this comment.
We have proper methods in parsec to output warnings. Please use parsec_warning instead.
There was a problem hiding this comment.
In general I don't think it is a good idea to return low level memory upstream, the ownership is unclear. As here we are talking about a very small amount of memory, I would suggest requiring the caller to provide an array of the size parsec_nb_devices into this call, and this function then does a single loop to fill this array up. All memory ownership is then on the caller size.
| dplasma_find_nb_devices(&args->gpu_device_index, &args->nb_gpu_devices); | ||
|
|
||
| /* Calculate the nested grid for the multiple GPUs on one process | ||
| * gpu_rows >= gpu_cols and as square as possible */ |
There was a problem hiding this comment.
the indentation is screwed. Please fix.
| @@ -18,7 +18,8 @@ int main(int argc, char ** argv) | |||
| { | |||
| parsec_context_t* parsec; | |||
| int iparam[IPARAM_SIZEOF]; | |||
| dplasma_enum_t uplo = dplasmaUpper; | |||
There was a problem hiding this comment.
Why do you map the A matrix on lower ? You don't need to alter this variable, instead just call the function with dplasmaLower and add a comment on why you choose to do so.
These are the performance comparisons.
I changed to Lower in Cholesky testing and will create an issue to track why Upper behaves differently.