jipolanco / PencilArrays.jl

Distributed Julia arrays using the MPI protocol
https://jipolanco.github.io/PencilArrays.jl/dev/
MIT License
60 stars 8 forks source link

Feature/add cuda #38

Closed corentin-dev closed 2 years ago

corentin-dev commented 2 years ago

This pull request is to add CUDA (or other AbstractVector) to PencilArrays. It is meant as a base for improvements.

It should create all the underlying structure with the good data types. I had some bugs with null pointers to CUDA arrays, that were solved with an ugly workaround (using a minimum resize of 1).

Indexing is still a problem for GPU, and it should be considered to use permdims in copy_range! for instance.

jipolanco commented 2 years ago

Thanks a lot for this work! It would be great if we could make things work with CUDA. Unfortunately I can't test on GPUs right now...

For now I have just some concerns regarding type stability, but I think these are easy to fix.

I also agree on the necessity of avoiding indexing for GPU arrays. Maybe, to avoid depending on CUDA, functions like copy_range! could have a default implementation (e.g. for AbstractVector) that avoids indexing, and a more specific implementation for CPU Vectors that uses indexing.

codecov-commenter commented 2 years ago

Codecov Report

Merging #38 (129ced5) into master (091d04f) will decrease coverage by 0.07%. The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage   97.03%   96.95%   -0.08%     
==========================================
  Files          17       17              
  Lines         978      986       +8     
==========================================
+ Hits          949      956       +7     
- Misses         29       30       +1     
Impacted Files Coverage Δ
src/arrays.jl 93.75% <80.00%> (-0.82%) :arrow_down:
src/Pencils/Pencils.jl 100.00% <100.00%> (ø)
src/Transpositions/Transpositions.jl 97.79% <100.00%> (-0.02%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 091d04f...129ced5. Read the comment docs.

jipolanco commented 2 years ago

I hope you don't mind, but I made a couple of changes to make sure that things are type stable. I also added some tests.

One of the things that I changed is the way the array type is specified in the Pencil constructor. Instead of taking a array_type keyword argument, now Pencil accepts an optional positional argument at the beginning that can be Array, CuArray, etc... For instance, this should work as expected:

p = Pencil(CuArray, (8, 10), MPI.COMM_WORLD)
u = PencilArray{Float32}(undef, p)

This way I could get things to be type stable, and in my opinion it's also simpler to use and more idiomatic (but this is subjective!).

I also changed the names of the eltype_* functions, which are no longer exported. (I didn't know about .name.wrapper, nice trick! Too bad that Julia doesn't export a function to do this). Some changes will be needed in your PencilFFTs PR, but I guess it's better if we finish this PR first.

corentin-dev commented 2 years ago

I hope you don't mind, but I made a couple of changes to make sure that things are type stable. I also added some tests.

Perfect!

One of the things that I changed is the way the array type is specified in the Pencil constructor. Instead of taking a array_type keyword argument, now Pencil accepts an optional positional argument at the beginning that can be Array, CuArray, etc... For instance, this should work as expected:

p = Pencil(CuArray, (8, 10), MPI.COMM_WORLD)
u = PencilArray{Float32}(undef, p)

This way I could get things to be type stable, and in my opinion it's also simpler to use and more idiomatic (but this is subjective!).

Yes, it makes sense. I did not want to change too much the code, but it is better this way.

I also changed the names of the eltype_* functions, which are no longer exported. (I didn't know about .name.wrapper, nice trick! Too bad that Julia doesn't export a function to do this). Some changes will be needed in your PencilFFTs PR, but I guess it's better if we finish this PR first.

Ok, I'll have to change some things in PencilFFTs since I used it for some change for CUDA too. I'll let you know.