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.

Sign up for GitHub

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

Jump to bottom

Fix CMake and CUDA compiler warnings #3859

Merged
merged 7 commits into from
Aug 14, 2020
Merged

Fix CMake and CUDA compiler warnings #3859

merged 7 commits into from
Aug 14, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Aug 14, 2020

Description of changes:

  • fix several issues in the CMake logic
  • fix function visibility issues in CUDA code (fixes Warnings in CUDA builds #3797)
  • document the CPU and GPU implementations of specfunc

jngrad added 5 commits August 14, 2020 15:40
@jngrad
core: Fix GCC -Werror=stringop-truncation
802c2d0 
Fixes the following compiler warning:
```
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:34: error:
'char* __builtin_strncpy(char*, const char*, long unsigned int)'
output may be truncated copying 64 bytes from a string of length 255
[-Werror=stringop-truncation]
```
The hard-coded value of 64 should probably be MPI_MAX_PROCESSOR_NAME,
but we cannot import mpi.h in CUDA code.
@jngrad
CMake: Guard compiler flag
85291b9
@jngrad
core: Fix function name collision in CUDA code
cf5940f 
mmm-common.hpp is no longer included in mmm-common_cuda.hpp. The
common parts (globals and prototypes) were moved to a new file
mmm-base.hpp. This fixes the following compiler warning:
```
Building NVCC (Device) object src/core/CMakeFiles/EspressoCore.dir/actor/EspressoCore_generated_Mmm1dgpuForce_cuda.cu.o
src/utils/include/utils/Span.hpp(88): warning: calling a __host__ function from a __host__ __device__ function is not allowed
          detected during instantiation of "Utils::Span<T>::Span(const C &) [with T=const double, C=std::vector<double, std::allocator<double>>, <unnamed>=std::vector<double, std::allocator<double>>, <unnamed>=std::vector<double, std::allocator<double>>]"
/home/espresso/espresso/src/core/electrostatics_magnetostatics/mmm-common.hpp(49): here
```
@jngrad
core: Document special functions
5079d3d
@jngrad
CMake: Fix incorrect CMake commands
23297be 
It is mandatory to provide a directory when listing CMake variables
with `cmake -L`, otherwise the command may fail. Since CMake 3.13.4,
running this command without a directory generates a warning. See
https://gitlab.kitware.com/cmake/cmake/-/issues/18838
@jngrad jngrad requested a review from KaiSzuttor August 14, 2020 13:54
@fweik
Copy link
Contributor

fweik commented Aug 14, 2020

Why did you rename mmm-common.cpp?

@jngrad
Copy link
Member Author

jngrad commented Aug 14, 2020

Why did you rename mmm-common.cpp?

I did not touch mmm-common.cpp. If you meant mmm-common.hpp, I had to move some of its content into a new file mmm-base.hpp that can be safely included in mmm-common_cuda.hpp. The CUDA compiler warning #3797 comes from the visibility of the CPU implementation of evaluateAsTaylorSeriesAt() in the CUDA code, which despite the different signature (2 vs. 3 arguments) caused nvcc to attempt instantiating the CPU function, even though it didn't need it. Furthermore the CUDA code doesn't need to know about the CPU implementation specfunc.hpp.

@fweik
Copy link
Contributor

fweik commented Aug 14, 2020

Ah sorry, I meant the header of course. mmm-base.hpp is pretty nondescript, if not misleading...

@jngrad
Copy link
Member Author

jngrad commented Aug 14, 2020

Ah sorry, I meant the header of course. mmm-base.hpp is pretty nondescript, if not misleading...

Do you have any suggestion? The file contains so little it's hard to come up with a good name. Maybe mmm-polygamma.hpp?

@fweik
Copy link
Contributor

fweik commented Aug 14, 2020

Also shouldn't the exported variables and functions be in a cpp file with the same name as the header?

@fweik
Copy link
Contributor

fweik commented Aug 14, 2020

I have a feeling that the stuff in mmm-base.hpp should be with the spec functions, if I understand the logic correctly. I'd just duplicate the code here, since the specfunction stuff is already duplicated.

@fweik
Copy link
Contributor

fweik commented Aug 14, 2020

Ceterum censeo... :-)

@jngrad
Copy link
Member Author

jngrad commented Aug 14, 2020

Also shouldn't the exported variables and functions be in a cpp file with the same name as the header?

Yes.

I have a feeling that the stuff in mmm-base.hpp should be with the spec functions, if I understand the logic correctly. I'd just duplicate the code here, since the specfunction stuff is already duplicated.

I would prefer not to duplicate any more code. I only realized today that specfunc and mmm-common were duplicated for GPU. Storing these duplicates in /src/core/actor/ instead of /src/core/electrostatics_magnetostatics/ seems like an anti-pattern to me.

I'll try to limit changes to specfunc here to make it easier to backport this PR to the 4.1 release. If we really need to refactor the code via duplication, we can discuss it again in a future PR that I'm working on to remove unused specfunc functions.

@fweik
Copy link
Contributor

fweik commented Aug 14, 2020

Well not a hill I'm willing to die on, but for the record I do think that duplication is the best solution given the circumstance.

@fweik
Copy link
Contributor

fweik commented Aug 14, 2020

Otherwise this looks good to me, except for the inline comments.

@jngrad
Copy link
Member Author

jngrad commented Aug 14, 2020

This refactoring is planned, see jngrad/espresso:specfunc-refactor. The goal of this PR is to fix compiler warnings.

@jngrad jngrad added the automerge Merge with kodiak label Aug 14, 2020
@kodiakhq kodiakhq bot merged commit c644d2d into espressomd:python Aug 14, 2020
@jngrad jngrad added this to the Espresso 4.1.4 milestone Aug 18, 2020
jngrad pushed a commit to jngrad/espresso that referenced this pull request Aug 18, 2020
@kodiakhq @jngrad
Fix CMake and CUDA compiler warnings (espressomd#3859)
80246da 
Description of changes:
- fix several issues in the CMake logic
- fix GCC compiler warning `-Werror=stringop-truncation` in GPU code
- document the CPU and GPU implementations of `specfunc`
jngrad pushed a commit to jngrad/espresso that referenced this pull request Aug 18, 2020
@kodiakhq @jngrad
Fix CMake and CUDA compiler warnings (espressomd#3859)
911763e 
Description of changes:
- fix several issues in the CMake logic
- fix GCC compiler warning `-Werror=stringop-truncation` in GPU code
- document the CPU and GPU implementations of `specfunc`
@jngrad jngrad deleted the fix-compiler-warnings branch January 18, 2022 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fweik fweik fweik approved these changes

@KaiSzuttor KaiSzuttor Awaiting requested review from KaiSzuttor

Assignees
No one assigned
Labels
automerge Merge with kodiak CMake Core Documentation
Projects
None yet
Milestone
Espresso 4.1.4
Development

Successfully merging this pull request may close these issues.

Warnings in CUDA builds
2 participants
@jngrad @fweik