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

Disable eigen vectorization by default. #1919

Merged
merged 3 commits into from Aug 28, 2017

Conversation

Projects
None yet
2 participants
@endJunction
Member

endJunction commented Aug 21, 2017

This needs testing on local machines ( @chleh @fparisio ).
Hopefully
fixes #1881

@endJunction endJunction force-pushed the endJunction:EigenDontVectorize branch 2 times, most recently from 3cf8004 to 7ec1db5 Aug 21, 2017

@chleh

Hopefully that will solve the issue for ever. I'll test this by next week.

IMHO now is the time to write some "How to improve OGS speed" tutorial. About fixed vs. dynamic matrices, vectorization (and issues with Eigen 3.3), shared libs, maybe also referring to some compile and runtime test results.
Who will do that?

@@ -143,7 +143,7 @@ TEST(MPITest_NumLib, ComponentNormMultiComponent1)
unsigned const num_components = 3;
auto const norm_type = MathLib::VecNormType::NORM1;
auto const tolerance =
num_components * 600 * std::numeric_limits<double>::epsilon();

This comment has been minimized.

@chleh

chleh Aug 22, 2017

Contributor

This is strange. Without vectorization I'd assume that the situation becomes better. But the resulting tolerance is still OK.

This comment has been minimized.

@endJunction

endJunction Aug 23, 2017

Member

Through the incremental increase this is now at 1700 * eps.

No vectorization also means that intermediate results can be stored (in registers) with 80bit precision, instead of 64 bit when using sse/avx.

This comment has been minimized.

@chleh

chleh Aug 24, 2017

Contributor

That sounds sound 😄 (Now the reference solution (computed with Eigen's routines) might use 80 bit at intermediate stages, whereas the component-wise computation only uses the 64 bit double precision everywhere.)

@@ -88,6 +88,7 @@ option(OGS_USE_PETSC "Use PETSc routines" OFF)
option(OGS_USE_EIGEN "Use Eigen linear solver" ON)
option(OGS_USE_EIGEN_UNSUPPORTED "Use Eigen unsupported modules" ON)
option(EIGEN_NO_DEBUG "Disables Eigen's assertions" OFF)
option(EIGEN_DONT_VECTORIZE "Disables explicit vectorization when defined." ON)

This comment has been minimized.

@chleh

chleh Aug 22, 2017

Contributor

I'd write:

# We assume that it's save to use vectorization with Eigen < 3.3 (strictly smaller than 3.3.!).
# At least we only observed vectorization issues with Eigen 3.3.x.
# If you want to use Eigen vectorization, make sure that you run all the ctests several times, e.g.:
# $ ctest --repeat-until-fail 50
# You might also want to take a look at https://github.com/ufz/ogs/issues/1881.
option(EIGEN_DONT_VECTORIZE "Disables explicit vectorization when defined. Please our cmake config files regarding the interplay of this and different Eigen versions!" ON)

Maybe you agree...

This comment has been minimized.

@endJunction

endJunction Aug 23, 2017

Member

Agree, but keeping the option's description as it is.

@endJunction endJunction force-pushed the endJunction:EigenDontVectorize branch 4 times, most recently from 58b1a5e to 5c378ac Aug 22, 2017

# If you want to use Eigen vectorization, make sure that you run all the ctests several times, e.g.:
# $ ctest --repeat-until-fail 50
# You might also want to take a look at https://github.com/ufz/ogs/issues/1881.
# Please see our cmake config files regarding the interplay of this and different Eigen versions!

This comment has been minimized.

@chleh

chleh Aug 24, 2017

Contributor

Thanks for adopting the comment. But IMHO the last line can be dropped now, since the one reading this comment has already dug into our cmake config, obviously.

@chleh

This comment has been minimized.

Contributor

chleh commented Aug 24, 2017

Once it runs: 👍

@chleh

This comment has been minimized.

Contributor

chleh commented Aug 24, 2017

Btw: What about the "How to: Performance tweaking" ?

@endJunction endJunction force-pushed the endJunction:EigenDontVectorize branch 2 times, most recently from 5174c90 to 5c378ac Aug 24, 2017

@endJunction

This comment has been minimized.

Member

endJunction commented Aug 24, 2017

@chleh @fparisio Now the tests seems to pass. I'd like to ask you to test this on the local machines especially for the test cases where this caused problems before.

@endJunction endJunction requested review from chleh and fparisio Aug 24, 2017

@chleh

This comment has been minimized.

Contributor

chleh commented Aug 28, 2017

For me the ctests now run. Please answer my two comments about

  • performance how to
  • the comment in the CMakeLists.txt
    Then feel free to merge from my side (maybe after review by second person.). 👍

@fparisio is on vacation this week AFAIK, so I think we should merge this without him having tested this PR, yet.

Further comment: Some of the ctests are very sensitive towards the number of OpenMP threads when solved with LIS, e.g., they fail with one thread, but succeed with either two or three threads.

@chleh

chleh approved these changes Aug 28, 2017

See my comment above.

@endJunction

This comment has been minimized.

@endJunction endJunction force-pushed the endJunction:EigenDontVectorize branch from 5c378ac to 5f9b2ad Aug 28, 2017

@endJunction

This comment has been minimized.

Member

endJunction commented Aug 28, 2017

@fparisio will test it in two weeks time.

endJunction added some commits Aug 21, 2017

[T] Relax component norm unit-test's tolerances.
This might relate to the different computation order
for vectorized vs. non-vectorized eigen computations.

@endJunction endJunction force-pushed the endJunction:EigenDontVectorize branch from 5f9b2ad to e2fd17a Aug 28, 2017

@endJunction endJunction merged commit 616f007 into ufz:master Aug 28, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@endJunction endJunction deleted the endJunction:EigenDontVectorize branch Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment