Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong pointer passed to Fortran MPI in FFTE #37

Closed
wants to merge 27 commits into from

Conversation

luszczek
Copy link
Contributor

This fixes the outstanding problem from #13 and shows how to call other functions from FFTE backend implementation.

@luszczek luszczek added the bug Something isn't working label Apr 10, 2022
@luszczek luszczek linked an issue Apr 10, 2022 that may be closed by this pull request
@luszczek luszczek assigned af-ayala and unassigned af-ayala Apr 10, 2022
@luszczek luszczek requested a review from af-ayala April 10, 2022 01:52
@luszczek
Copy link
Contributor Author

luszczek commented Apr 10, 2022

CI seems to be saying something different than my build. I've run the FFTE test with the same set of packages and got no issues. It seems that the build inside CI doesn't get the latest patch and runs the old code that segfaulted before. Here's my output:

mpirun -np 2 test3D_C2C -lib ffte -backend fftw -size 4 4 4 -pgrid 1 2
-----------------------------------------------
Benchmarking ffte library using fftw backend
-----------------------------------------------
[0] proc_grid 1x1x2 - [New MPI process 0] I am located at (0, 0).   0-0-0    3-3-1   local_size={32}
[ffte] successfully initialized.
[1] proc_grid 1x1x2 - [New MPI process 1] I am located at (0, 1).   0-0-2    3-3-3   local_size={32}
Benchmarking FFTE: Get Complex-to-Complex backend using ptest3d.f
Benchmarking FFTE: Get Complex-to-Complex backend using ptest3d.f
Time for FFT plan  = 2.3956e-05
Time for execution =    333.864
Benchmarking FFTE: Get Complex-to-Complex backend using ptest3d.f
Benchmarking FFTE: Get Complex-to-Complex backend using ptest3d.f
FFTE: |X - ifft(fft(X)) |_{max}: 3.051562e+01

@G-Ragghianti
Copy link
Contributor

You are correct. The CI uninstalls the fiber package at the begining of the job, but doesn't touch the previously installed dependencies. I will update the CI so that it will uninstall all the FFT libraries as well.

CI seems to be saying something different than my build. I've run the FFTE test with the same set of packages and got no issues. It seems that the build inside CI doesn't get the latest patch and runs the old code that segfaulted before. Here's my output:

@G-Ragghianti
Copy link
Contributor

Correction: The CI is supposed to be uninstalling the fiber and fft packages, but it doesn't appear to be doing so. I'm investigating the cause.

@G-Ragghianti
Copy link
Contributor

OK, the CI is now able to uninstall fiber and the fft library before attempting to install and test. I will run the CI checks on this PR.

@@ -0,0 +1,50 @@
cmake_minimum_required(VERSION 3.8)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For uniformity, I would suggest to keep the same 3.10 version as in the main CMakeList?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not for FIBER. This is for those who build FFTE with CMake. FFTE provides only Makefile's for their tests and nothing else. FFTE doesn't need a new CMake version than 3.8 that has features for libraries and for CUDA language.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed d5d4896 to make them uniform

@G-Ragghianti
Copy link
Contributor

Can you merge the changes that I recently made to the CI into your branch in order for the test to run with the updated CI config? The changes are under .github/

@luszczek
Copy link
Contributor Author

It took me forever to merge your CI branch. I hope it was worth it.

@luszczek
Copy link
Contributor Author

I'd be okay to close this and start from scratch after the tests finish.

@luszczek
Copy link
Contributor Author

Closing due to wrong rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FFTE backend
3 participants