espressomd / espresso

The ESPResSo package
https://espressomd.org
GNU General Public License v3.0
225 stars 183 forks source link

P3M: further FFt refactoring #4963

Closed RudolfWeeber closed 2 weeks ago

RudolfWeeber commented 1 month ago

To my understanding, the FFT can be refactored furhter:

move out unrelated halo communicatoin

The FFTBackend currently calls the halo communication before/after the FFT. However, to my understanding, this halo communication is unrelated to the FFT. It should be done by the actual P3M classes.

Background: the halo communication is needed when interpolationg particle<->mesh. A particle sitting at the boundary needs to be able to read/write mesh sites across the boundary, so there is a halo layer. to my understanding, this is all in real space.

Making explicit what is being transformed

To my understanding, we have the following FFTs

IMO, it would be clearer, if the backend API provides these 4 functions, and the respective P3M implementations pass as arguments what they want transformed.

jngrad commented 1 month ago

The FFTBackend currently calls the halo communication before/after the FFT.

Can you show me an example? As far as I know, fft.cpp and FFTBackendLegacy.cpp have no knowledge of the particles and cell structure:

/work/jgrad/espresso/src/core/p3m/FFTBackendLegacy.cpp:112:30: error: ‘Particle’ was not declared in this scope
  112 | auto constexpr num1 = sizeof(Particle);
/work/jgrad/espresso/src/core/p3m/FFTBackendLegacy.cpp:113:30: error: ‘CellStructure’ was not declared in this scope; did you mean ‘CellStructureType’?
  113 | auto constexpr num2 = sizeof(CellStructure);

IMO, it would be clearer, if the backend API provides these 4 functions, and the respective P3M implementations pass as arguments what they want transformed.

We can have 4 FFT methods. The new API would look like this:

// src/core/fft/fft.hpp
struct fft_data_struct {
  void forward_fft(boost::mpi::communicator const &comm, double *data);
  void backward_fft(boost::mpi::communicator const &comm, double *data, bool check_complex);
};

// src/core/p3m/data_struct.hpp
class FFTBackend {
  std::unique_ptr<fft_data_struct> fft;
  P3MFFTMesh &mesh;
  void update_mesh_data();
public:
  virtual void perform_scalar_fwd_fft() = 0;
  virtual void perform_vector_fwd_fft() = 0;
  virtual void perform_scalar_back_fft() = 0;
  virtual void perform_vector_back_fft() = 0;
};

The 4 FFT methods in FFTBackend don't take arguments, because none of the P3M implementations we support (P3M, P3MGPU, DipolarP3M) need to know how the FFT buffers are stored in memory. Could be std::vector, fft::vector, or Kokkos containers. Once a transform is complete, method update_mesh_data() copies the transformed data to the corresponding destination buffer of the P3M object via handle mesh.

RudolfWeeber commented 1 month ago

Halo communication: I was not referring to the particle halo communication but ot the one of P3M, i.e., the send mesh business. If a particle sits at the MPI rank's somain bodunary, charge from it is assigned to mesh sites outside that domain. This is then halo communicated and added to the neighboring MPI rank's corresponding mesh sites. Similarly, once the potential/field is calculated, the contribution of the field to each particle's force is obtained by back-interpolating from the mesh to the particle. For that, a particle sitting at the domain boundary has to have access to mesh sites of the neighboring MPI rank.

To my understanding, this is unrelated to the 3d FFT. However, the calls to gather_grid and spread_grid currently are in the FFtBackendLegacy.

jngrad commented 1 month ago

To be honest, I had no clue what the send mesh code was doing. It has hardcoded calls to functions from our legacy FFT algorithm, so it felt natural to make it part of the FFTBackendLegacy class. Extracting the logic will be a challenge.

RudolfWeeber commented 1 month ago

Arguments to FFTs and data types: I see. We need to revisit this, though. The dipolar P3M iterates over the entire mesh several times right now, as it has to re-use the array for the back-transofrm several times for different things. That could be avoided if the FFTs just took arguments.

Maybe sth like

auto result = from_fft_vector(forward_fft(to_fft_vector(data))

would be possible. The conversion funcitons can then decide, whether a copy is needed or, e.g.,, just return the ->data() member of a std::vector.