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

Collected small changes for the next patch #1492

Merged
merged 9 commits into from Jun 7, 2019

Conversation

Projects
None yet
2 participants
@akohlmey
Copy link
Member

commented Jun 5, 2019

Summary

This pull request combines a variety of small changes and bugfixes

Related Issues

This closes #1403
This supersedes and closes #1491

Author(s)

Axel Kohlmeyer (Temple U)

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility

yes, for valid inputs using kspace_modify mesh and kspace_modify mesh/disp. Incorrect inputs that may have been accepted before will now be rejected.

Implementation Notes

The following individual changes are included:

  • nicer formatting of the tables of styles
  • require all zero or fully valid inputs of kspace meshes for coulomb and dispersion
  • print the message "Gathering installed package information (may take a little while)" earlier in the conventional build process and thus properly inform people on slow file system, that things may be slow, before and slow - because of lots of i/o - operations starts.
  • more accurate matching for styles using utils::strmatch() so that styles that differ by suffixes are matched correctly as well.
  • try to turn off optimization for functions that build "factories" as they can take an excessive time to compile with full optimization for no gain. also for GCC >= 4.4 turn of variable tracking and thus avoid a confusing warning about overflowing the pre-defined space for it.

Post Submission Checklist

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system
  • A package specific README file has been included or updated
  • One or more example input decks are included

akohlmey added some commits Jun 5, 2019

@akohlmey akohlmey added this to the Stable Release Summer 2019 milestone Jun 5, 2019

@akohlmey akohlmey self-assigned this Jun 5, 2019

@stanmoore1 stanmoore1 self-requested a review Jun 5, 2019

@@ -443,15 +443,23 @@ void KSpace::modify_params(int narg, char **arg)
nx_pppm = nx_msm_max = force->inumeric(FLERR,arg[iarg+1]);
ny_pppm = ny_msm_max = force->inumeric(FLERR,arg[iarg+2]);
nz_pppm = nz_msm_max = force->inumeric(FLERR,arg[iarg+3]);
if (nx_pppm == 0 && ny_pppm == 0 && nz_pppm == 0) gridflag = 0;
if (nx_pppm == 0 && ny_pppm == 0 && nz_pppm == 0)

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 5, 2019

Contributor

I don't think we need this check. The default (if no kspace_modify mesh is given) is to set the grid automatically, so I don't think a user would go to the trouble of explicitly setting the grid to all zeros using a kspace_modify command, just to get the default behavior.

This comment has been minimized.

Copy link
@akohlmey

akohlmey Jun 5, 2019

Author Member

you never know what is going to happen in the future and what people are going to try and then get confused by inconsistent behavior of a program. this extra test doesn't cost us any performance and makes certain, that settings are always consistent and people can switch around as they like.

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 5, 2019

Contributor

Is this documented anywhere though?

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 5, 2019

Contributor

Yes it is documented in kspace_modify, "Values for x,y,z of 0,0,0 unset the option." So you are right, we should leave it.

akohlmey added some commits Jun 5, 2019

turn off variable tracking through turning off optimization for GCC 4…
….4 and later

This will avoid a difficult to interpret warning and in
addition speed up compilation of this one file by avoiding
to try to optimize something, that needs no optimization.
@@ -53,6 +53,12 @@
#include "memory.h"
#include "error.h"

#if defined(__GNUC__) && !defined(__clang__) && !defined(__INTEL_COMPILER)
#if (__GNUC__ > 4) || ((__GNUC__ == 4) && (__GNUC_MINOR__ >= 4))
#pragma GCC optimize ("O0")

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 6, 2019

Contributor

@akohlmey have you checked to see if this has any performance loss?

This comment has been minimized.

Copy link
@akohlmey

akohlmey Jun 6, 2019

Author Member

how could there be any? this only affects the compilation of the body of the LAMMPS class and all it does is create other class instances and handle command line flags.

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 6, 2019

Contributor

Some compilers support whole-program interprocedural optimization. So turning off all optimizations makes me nervous.

This comment has been minimized.

Copy link
@akohlmey

akohlmey Jun 6, 2019

Author Member
  1. this applies to GCC only, so it doesn't really matter what other compilers do.
  2. this pragma is documented to affect only the statements following it. if GCC would propagate this setting to other files, it would be a bug in GCC.

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 6, 2019

Contributor

Well:

  1. GCC does support both interprocedural optimization and link time optimization, through the flags -fwhole-program --combine, and -flto.
  2. I'm not saying it would propagate the -O0 setting to other files, but I am saying it might prevent whole-program optimization from occurring.

Is it just a warning you trying to fix by this? This seems like smashing the ant with a pretty big hammer.

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 6, 2019

Contributor

I think this needs to at least use #pragma GCC push/pop options, or a volatile statement.

This comment has been minimized.

Copy link
@akohlmey

akohlmey Jun 6, 2019

Author Member

From the GCC docs:

'#pragma GCC push_options'
'#pragma GCC pop_options'

     These pragmas maintain a stack of the current target and
     optimization options.  It is intended for include files where you
     temporarily want to switch to using a different '#pragma GCC
     target' or '#pragma GCC optimize' and then to pop back to the
     previous options.

"volatile statement" where?

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 6, 2019

Contributor

@akohlmey I meant something like this. It compiles very fast, optimizes -O3 everywhere but the single function, and doesn't have the warning:

diff --git a/src/lammps.cpp b/src/lammps.cpp
index 780327c..adaa2b2 100644
--- a/src/lammps.cpp
+++ b/src/lammps.cpp
@@ -55,8 +55,10 @@

 #if defined(__GNUC__) && !defined(__clang__) && !defined(__INTEL_COMPILER)
 #if (__GNUC__ > 4) || ((__GNUC__ == 4) && (__GNUC_MINOR__ >= 4))
-#pragma GCC optimize ("no-var-tracking-assignments")
+#define OPTFLAG __attribute__((optimize("O0"))) __attribute__((optimize("no-var-tracking-assignments")))
 #endif
+#else
+#define OPTFLAG ""
 #endif

 #include "lmpinstalledpkgs.h"
@@ -902,7 +904,7 @@ void LAMMPS::destroy()
    initialize lists of styles in packages
 ------------------------------------------------------------------------- */

-void LAMMPS::init_pkg_lists()
+void OPTFLAG LAMMPS::init_pkg_lists()
 {
   pkg_lists = new package_styles_lists;
 #define PACKAGE "UNKNOWN"

This comment has been minimized.

Copy link
@akohlmey

akohlmey Jun 6, 2019

Author Member

if we're going this far, we can also apply it to the equivalent functions in force.cpp and modify.cpp that create instances of styles.

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 6, 2019

Contributor

@akohlmey sounds good.

@@ -53,6 +53,12 @@
#include "memory.h"
#include "error.h"

#if defined(__GNUC__) && !defined(__clang__) && !defined(__INTEL_COMPILER)
#if (__GNUC__ > 4) || ((__GNUC__ == 4) && (__GNUC_MINOR__ >= 4))
#pragma GCC optimize ("O0")

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 6, 2019

Contributor

I think this needs to at least use #pragma GCC push/pop options, or a volatile statement.

akohlmey added some commits Jun 6, 2019

disable optimization on functions building factories for many entries
this will speed up compilation and also avoid spurious warnings with gcc 4.4 and later

@akohlmey akohlmey requested a review from stanmoore1 Jun 7, 2019

@akohlmey akohlmey marked this pull request as ready for review Jun 7, 2019

@akohlmey akohlmey requested a review from sjplimp as a code owner Jun 7, 2019

@akohlmey akohlmey merged commit b9e10d5 into lammps:master Jun 7, 2019

6 checks passed

lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/cmake/cmake-serial-pr head run ended
Details
lammps/pull-requests/kokkos-omp-pr head run ended
Details
lammps/pull-requests/openmpi-pr head run ended
Details
lammps/pull-requests/serial-pr head run ended
Details
lammps/pull-requests/shlib-pr head run ended
Details

#if defined(__clang__)
# define _noopt __attribute__((optnone))
#elif defined(__INTEL_COMPILER)

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 7, 2019

Contributor

@akohlmey It looks like the Intel compiler doesn't support attributes; instead of attributes, one can also use:

#pragma GCC push_options
#pragma GCC optimize ("-O0")
void foo()
{
   ...
}
#pragma GCC pop_options

And many more compilers like the Intel compiler and clang support something similar to this as well, see https://stackoverflow.com/questions/31373885/how-to-change-optimization-level-of-one-function.

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 7, 2019

Contributor

But probably not worth the trouble...

@akohlmey akohlmey deleted the akohlmey:collected-small-changes branch Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.