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

turn on parallel Eigen #793

Open
paciorek opened this issue Oct 4, 2018 · 7 comments
Open

turn on parallel Eigen #793

paciorek opened this issue Oct 4, 2018 · 7 comments

Comments

@paciorek
Copy link
Contributor

paciorek commented Oct 4, 2018

We should do this for next release.

@paciorek
Copy link
Contributor Author

paciorek commented Nov 12, 2018

In the parallel_eigen branch, I've inserted -fopenmp in nimble/inst/make/*.in.

I don't think we need to insert -fopenmp in src/Makevars*in or inst/include/CppCode/Makevars as I don't think we have any Eigen-based linear algebra in our hard-coded C++, but @perrydv let me know if you have other thoughts.

OTOH, there is presumably no downside to enabling -fopenmp for {lib,}nimble.so so we might do it just in case there is some such code in the future.

@paciorek
Copy link
Contributor Author

One other comment is that Eigen's parallelization seems limited. As of Eigen 3.3.5:

Currently, the following algorithms can make use of multi-threading:

    general dense matrix - matrix products
    PartialPivLU
    row-major-sparse * dense vector/matrix products
    ConjugateGradient with Lower|Upper as the UpLo template parameter.
    BiCGSTAB with a row-major sparse matrix format.
    LeastSquaresConjugateGradient

So turning on parallelization will probably not help end users by and large.

@paciorek
Copy link
Contributor Author

Testing indicates an issue with -fopenmp on macOS.

More info:
https://stackoverflow.com/questions/42280628/requiring-openmp-availability-for-use-in-an-rcpp-package/42281495#42281495

Also not clear that -fopenmp would by default be supported on Windows.

Writing R extensions section 1.2.11 indicates one should use SHLIB_OPENMP_CXXFLAGS in Makevars* files.

I've now implemented this, but on one of my Macs, -fopenmp is being injected despite seeming to not be supported. Perhaps this means R was configured incorrectly (though I think I did a basic R install on that machine). But we should be cautious here for fear of breaking users' use of nimble if they don't have openMP support.

Side note: users can also do this at run time if their compiler supports openMP:

Sys.setenv("PKG_CXXFLAGS"="-fopenmp")
Sys.setenv("PKG_LIBS"="-fopenmp")

Cautionary note from Writing R extensions: Note that the macro SHLIB_OPENMP_CXXFLAGS applies to the default C++ compiler and not necessarily to the C++11/14/17 compiler: users of the latter should do their own configure checks (an example is available in CRAN package ARTP2).

@paciorek
Copy link
Contributor Author

paciorek commented Nov 20, 2018

Ok, so this is failing on two separate Macs. -fopenmp is being injected into the clang++ call and that is erroring out with an "unsupported option '-fopenmp'" error.

This thread seems to be relevant, but I'm not understanding everything there (in particular the discussion is really about Travis).
jonclayden/mmand#4

@paciorek
Copy link
Contributor Author

On SCF Mac, SHLIB_OPENMP_CXXFLAGS seems to be set to "-fopenmp" in:

/Library/Frameworks/R.framework/Versions/3.4/Resources/etc/Makeconf

Unfortunately if one then sets the following in a local Makevars:

PKG_CPPFLAGS= -DR_NO_REMAP  -Wno-misleading-indentation -Wno-ignored-attributes -Wno-deprecated-declarations
PKG_CXXFLAGS=$(SHLIB_OPENMP_CXXFLAGS)
PKG_LIBS= $(SHLIB_OPENMP_CXXFLAGS) $(LAPACK_LIBS) $(BLAS_LIBS) 
CXX_STD = CXX11

as per Nimble's inst/make/Makevars, then compilation fails.

Unclear why "-fopenmp" is set given openMP seemingly can't be successfully used.

@paciorek
Copy link
Contributor Author

paciorek commented Nov 21, 2018

My current thought is that at run-time we want to set -fopenmp if openMP is available to user via the local Makevars in nimble_generatedCode. Why?

  1. it seems that SHLIB_OPENMP_CXXFLAGS could be set in a user's R such that it causes compilation failures. So we can't rely on the value it is set to.
  2. Most Mac/Win users will get the binary package so we have no ability to monkey with nimble's inst/make/Makevars as it is placed on a user's machine.
    2a) Any attempt to monkey with nimble's inst/make/Makevars is done at package build time and for Mac/Win users this is based on CRAN's build machine.

Plan - look at how dynamicRegistrations stuff is done and how we write Makevars in nimble_generatedCode. We might set a nimbleOption the first time compilation is done based on querying the user's system to see if -fopenmp can be used and then at subsequent times in a given R session, use the nimbleOption.

Not clear on the following:

  • do we hard-code SHLIB_OPENMP_CXXFLAGS into inst/make/Makevars* and then modify SHLIB_OPENMP_CXXFLAGS to set it to -fopenmp in nim_genCode Makevars (seems like danger could be that if we set it wrong then we might get the problematic value of SHLIB_OPENMP_CXXFLAGS from user system.
  • alternatively, in nim_genCode Makevars, we might put -fopenmp into PKG_CXXFLAGS and PKG_LIBS (note even if this is done before the include of inst/make/Makevars, it does seem to be picked up along with the values of these flags in inst/make/Makevars)
  • or we might just insert the string "-fopenmp" into the system() call that does R CMD SHLIB and avoid the nim_genCode Makevars entirely.

@paciorek
Copy link
Contributor Author

It would probably be good to allow for the nimbleOption to behave such that users could turn on or off -fopenmp as they prefer and we won't override that. So the options might be: auto/on/off where auto is that we detect. So there might be a user-facing option and then the option that actually says whether openMP is on for a given user session so we don't have to keep testing whether -fopenmp works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant