Project

General

Profile

Bug #2503

post-submit has warnings

Added by Roland Schulz over 1 year ago. Updated 11 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Associated revisions

Revision d181859f (diff)
Added by Mark Abraham 12 months ago

Move responsibility for PME reduction to its module

We no longer need temporary energy and virial variables that gcc 7 and
8 warn about in release mode. Earlier efforts to avoid these warnings
made pme.h depend on config.h, which is not desirable.

This made possible minor simplifications to testing code.

Noted some TODOs and added some comments. Removed a completed TODO
that had been left behind.

Fixes #2503
Refs #2863

Change-Id: I9a6c5b12ef5c27bd003d3ab9eeeaa75e9574b2dc

History

#1 Updated by Mark Abraham over 1 year ago

We know. There's been some attempts to fix it (e.g. disable running tests, introduce getters such as those reverted in that commit). I've even been working on the kind of better structured reduction that I proposed back in December instead of the version that is currently there. But it's just not a priority this moment.

#2 Updated by Mark Abraham over 1 year ago

  • Status changed from New to Accepted

#3 Updated by Szilárd Páll over 1 year ago

Yeah, no relaese builds with gcc >= 7 (or 6) are warning-free.

Proposal has been to re-introduce the code reverted to fix warnings (in particular as the revert assumed it was only a performance improvement), but the proposal was not received very well: https://gerrit.gromacs.org/#/c/7846/

#4 Updated by Mark Abraham over 1 year ago

I can't reproduce the sim_util.cpp issue with release mode gcc 7 or gcc 8 on x86. gcc 7 release mode built warning free for me. Release with gcc 8 has a host of string handling warnings (at least, and of course those warnings are not dependent the build type).

#5 Updated by Aleksei Iupinov over 1 year ago

I assume you've set -DGMX_GPU=OFF? Otherwise, I can only think of -DGMX_COMPILER_WARNINGS=ON.

#6 Updated by Mark Abraham over 1 year ago

Aleksei Iupinov wrote:

I assume you've set -DGMX_GPU=OFF?

Yes, because that's what the post-submit config does (but I also see no warnings in release with master, CUDA 9.2 and gcc 7.1, so it's hard to see where Szilard's claim comes from). I assume the armhpc gcc might be doing something atypical in the configuration of its gcc, but the issue in our code is worth solving (properly, without creating another issue).

Otherwise, I can only think of -DGMX_COMPILER_WARNINGS=ON.

Yes I always use -DGMX_DEVELOPER_BUILD=on which has that effect also.

#7 Updated by Szilárd Páll over 1 year ago

$ module load cmake/3.9.4 fftw/3.3.7-sse2-avx-avx2-avx128fma-avx512 hwloc/1.11.6 gcc/7.3
[...]
$ CC=gcc-7 CXX=g++-7 cmake ../ -DCMAKE_PREFIX_PATH="${FFTW_HOME}" -DGMX_CYCLE_SUBCOUNTERS=ON -DGMX_GPU=ON   -DGMX_USE_OPENCL=ON && make -j24

[...]
[ 92%] Building CXX object src/gromacs/CMakeFiles/libgromacs.dir/ewald/pme-pp.cpp.o
In file included from /tmp/gromacs-master/src/gromacs/mdlib/sim_util.cpp:88:0:
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h: In function ‘void do_force(FILE*, const t_commrec*, const gmx_multisim_t*, const t_inputrec*, gmx_int64_t, t_nrnb*, gmx_wallcycle_t, gmx_localtop_t*, const gmx_groups_t*, real (*)[3], gmx::PaddedArrayRef<gmx::BasicVector<float> >, history_t*, gmx::PaddedArrayRef<gmx::BasicVector<float> >, real (*)[3], const t_mdatoms*, gmx_enerdata_t*, t_fcdata*, gmx::ArrayRef<float>, t_graph*, t_forcerec*, const gmx_vsite_t*, real*, double, const gmx_edsam*, int, DdOpenBalanceRegionBeforeForceComputation, DdCloseBalanceRegionAfterForceComputation)’:
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h:103:65: warning: ‘vir_Q[0]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
                         virial_[dim1][dim2] += virial[dim1][dim2];
                                                ~~~~~~~~~~~~~~~~~^
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h:103:65: warning: ‘vir_Q[1]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h:103:65: warning: ‘vir_Q[2]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h:103:65: warning: ‘*((void*)(& vir_Q)+12)[0]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h:103:65: warning: ‘*((void*)(& vir_Q)+12)[1]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h:103:65: warning: ‘*((void*)(& vir_Q)+12)[2]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h:103:65: warning: ‘*((void*)(& vir_Q)+24)[0]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h:103:65: warning: ‘*((void*)(& vir_Q)+24)[1]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/tmp/gromacs-master/src/gromacs/mdtypes/forceoutput.h:103:65: warning: ‘*((void*)(& vir_Q)+24)[2]’ may be used uninitialized in this function [-Wmaybe-uninitialized]

[...]

[ 95%] Building CXX object src/gromacs/CMakeFiles/libgromacs.dir/mdrun/tpi.cpp.o
/tmp/gromacs-master/src/gromacs/ewald/pme-gpu-3dfft-ocl.cpp: In member function ‘void GpuParallel3dFft::perform3dFft(gmx_fft_direction)’:
/tmp/gromacs-master/src/gromacs/ewald/pme-gpu-3dfft-ocl.cpp:168:21: warning: ‘outputGrids’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     handleClfftError(clfftEnqueueTransform(plan, direction,
     ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                            commandStreams_.size(), commandStreams_.data(),
                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                            waitEvents.size(), waitEvents.data(), outEvents,
                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                            inputGrids, outputGrids, tempBuffer), "clFFT execution failure");
                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/gromacs-master/src/gromacs/ewald/pme-gpu-3dfft-ocl.cpp:168:21: warning: ‘inputGrids’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/tmp/gromacs-master/src/gromacs/ewald/pme-gpu-3dfft-ocl.cpp:168:21: warning: ‘direction’ may be used uninitialized in this function [-Wmaybe-uninitialized]
/tmp/gromacs-master/src/gromacs/ewald/pme-gpu-3dfft-ocl.cpp:168:21: warning: ‘plan’ may be used uninitialized in this function [-Wmaybe-uninitialized]

And now a few more warnings.

#8 Updated by Mark Abraham over 1 year ago

I can now reproduce the warnings with gcc 7.1 and 7.3 on x86 in release mode for an OpenCL or no-GPU build config (as expected). A CUDA build is warning free everywhere, of course. Apparently I was doing something differently when I tried earlier.

I have a patch in preparation that refactors the reduction code to make these unused variables compile away, but it's not yet in shape for review.

#9 Updated by Mark Abraham 12 months ago

  • Target version set to 2019

#10 Updated by Gerrit Code Review Bot 12 months ago

Gerrit received a related patchset '1' for Issue #2503.
Uploader: Mark Abraham ()
Change-Id: gromacs~release-2019~I9a6c5b12ef5c27bd003d3ab9eeeaa75e9574b2dc
Gerrit URL: https://gerrit.gromacs.org/8757

#11 Updated by Mark Abraham 12 months ago

  • Status changed from Accepted to Fix uploaded

#12 Updated by Mark Abraham 12 months ago

  • Status changed from Fix uploaded to Resolved

#13 Updated by Paul Bauer 11 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF