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

Add GH Actions runners for mingw and Intel compiler #1144

Merged
merged 7 commits into from
Dec 8, 2021

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Nov 25, 2021

Changes proposed in this pull request

  • Add GH runner for mingw toolchain (on Windows)
  • Add GH runner for icc compiler (on Ubuntu)
  • Add minor updates for compiler flags
  • New intel compilers icx/icpx are recognized, but not tested (to be more exact: compilation works, but tests segfault) … compilers work as long as optimization is disabled.
  • Add GH runner for icx compiler (on Ubuntu)

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#117, closes Cantera/enhancements#120, closes #1150.

If applicable, provide an example illustrating new features this pull request is introducing

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

Other

FWIW, 'out-of-the-box', the following tests are failing:

For mingw:

Failed tests:
    - thermo: cubicSolver_Test.solve_cubic

For icc:

Failed tests:
    - VCS-LiSi-verbose
    - cxx-kinetics1

... while icc unit tests can be addressed, no action is taken as icc wil be replaced by a new compiler generation icx (which does not need modifications)

Also, I created a docker container for testing, see here

@ischoegl
Copy link
Member Author

ischoegl commented Nov 26, 2021

@bryanwweber / @speth ... I think this is as far as I can go with reasonable effort.

I had a closer look at the failing tests, and believe the only reason were minor numerical differences between Intel MKL (?) calculations and what is used elsewhere. I loosened the tolerances for cxx-kinetics1(same 'looser' tolerance that is used in other tests), and skipped the verbose test VCS-LiSi-verbose for lack of better options (verbose output is generated from a different execution branch when equilibrating, resulting in a different log output).

One thing that I briefly looked at, but decided to leave for a later point is pch (precompiled headers), which had already been disabled. I realize that I am not familiar enough with scons to find the correct flags; as far as I was able to find out, scons should actually recognize and use the correct options for pch. But at least for icc I am stuck with a Warning #672: the command line options do not match those used when precompiled header file "src/pch/system.pchi" was created.

Beyond, I put in some rudimentary information related to Intel's new icx/icpx/ifx compilers. While cantera builds (even with pch!) things segfault during testing.

@ischoegl ischoegl marked this pull request as ready for review November 26, 2021 17:32
@ischoegl ischoegl requested a review from a team November 26, 2021 17:32
@ischoegl ischoegl force-pushed the compile-with-icc branch 3 times, most recently from 3557db2 to 484b6cd Compare November 26, 2021 23:31
@ischoegl ischoegl changed the title Add GH Actions runner for intel compiler Add GH Actions runner for mingw and intel compiler Nov 27, 2021
@ischoegl
Copy link
Member Author

ischoegl commented Nov 27, 2021

I had a half-finished mingw GH runner laying around, and just gave it another shot - as it appears to work, I am adding it here.

@gkogekar ... interestingly, one of the solveCubic tests failed: see band-aid in commit d8b3fc8. If I interpret this correctly, a different root is found for mingw which may or may not create issues for PengRobinson.

@ischoegl ischoegl changed the title Add GH Actions runner for mingw and intel compiler Add GH Actions runners for mingw and Intel compiler Nov 27, 2021
@ischoegl

This comment has been minimized.

@ischoegl
Copy link
Member Author

ischoegl commented Nov 27, 2021

Ad icc/icpc: changing the Wcheck (perform compile-time code checking for certain code) to Wall (enable warning and error diagnostics) option gets rid of most compiler warnings that are due to fmt. The same isn't true for icx/icpx (new Intel compilers), where both fmt 6.2.1 and 8.0.1 cause issues. (Edit: fwiw, I reported this as fmtlib/fmt#2619, which however is already closed)

@ischoegl ischoegl marked this pull request as ready for review November 27, 2021 16:02
@ischoegl ischoegl force-pushed the compile-with-icc branch 5 times, most recently from 16d0313 to ea895b6 Compare November 28, 2021 19:09
@ischoegl

This comment has been minimized.

SConstruct Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Dec 6, 2021

@bryanwweber ... I think #1150 shed a few insights here. I dropped some of the commits that were not essential, and added 'experimental' support for icx: as long as optimization is disabled, things work. With that, I believe what is here is ready for review.

@tpg2114
Copy link

tpg2114 commented Dec 7, 2021

Just a thought -- you might not need to relax the tolerance on cxx-kinetics1 with the classic Intel compilers if the fp-model strict flag was added to that compiler set too. I don't know how important the tolerance and accuracy is for that case, but it might be worth checking (and deciding on importance).

@ischoegl ischoegl force-pushed the compile-with-icc branch 3 times, most recently from af17006 to 60fa7f7 Compare December 7, 2021 23:40
Add icx to list of recognized compilers: while code compiles, unit tests
result in segfaults, which can be avoided if optimization is disabled.

Add pch for icx compilation
Co-authored-by: @tpg2114

Optimized compilation works if options include '-fp-model strict'
(or 'precise'). Segfaults can be reproduced with gcc if option
'-ffinite-math-only' is selected.
@ischoegl
Copy link
Member Author

ischoegl commented Dec 8, 2021

@bryanwweber and @tpg2114 ... as the compilation issues are apparently not compiler related (see new issue #1155), I believe that Intel OneAPI can be added to the list of fully supported compilers at this point (as long as the required optimization settings are applied). I consolidated all commits and replaced the GH runner for icc by one for icx, where the previous 'fixes' for tolerances are no longer needed. As compilation with icx works (including pch support), this PR will close #1150, with the -ffast-math becoming a separate issue (which may or may not be worth pursuing).

I left the mingw runner in, but can drop it (or make a separate PR) in case the one failing test (where a band-aid is applied) needs further investigations.

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ischoegl! A few small comments here.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
SConstruct Show resolved Hide resolved
SConstruct Show resolved Hide resolved
interfaces/cython/SConscript Outdated Show resolved Hide resolved
test/thermo/cubicSolver_Test.cpp Outdated Show resolved Hide resolved
@speth
Copy link
Member

speth commented Dec 8, 2021

I left the mingw runner in, but can drop it (or make a separate PR) in case the one failing test (where a band-aid is applied) needs further investigations.

I would suggest leaving the mingw compiler in, but I think the inconsistent return value arising from MixtureFugacityTP shouldn't be ignored by just making the test succeed if the result is either +2 or -2. The PengRobinson and RedlichKwong models both use this value and do not treat the positive and negative return values equally. I can create a new issue for this so it doesn't get lost.

For this PR, I would suggest skipping this test if the __MINGW32__ preprocessor define is present, e.g. something like:

#ifdef __MINGW32__
TEST_F(cubicSolver_Test, solve_cubic_DISABLED)
#else
TEST_F(cubicSolver_Test, solve_cubic)
#endif

@ischoegl ischoegl force-pushed the compile-with-icc branch 3 times, most recently from 87782d2 to 398d0d8 Compare December 8, 2021 16:35
@ischoegl
Copy link
Member Author

ischoegl commented Dec 8, 2021

@bryanwweber ... thanks for the review!

@speth ... thanks for the suggestion!

The PengRobinson and RedlichKwong models both use this value and do not treat the positive and negative return values equally. I can create a new issue for this so it doesn't get lost.

I excluded the test as suggested, but am not familiar enough to create an issue report. I tried tagging @gkogekar as this presumably is an edge case introduced by the PengRobinson PR #1047. FWIW, this is the output for the time being

[----------] 1 test from cubicSolver_Test
[ RUN      ] cubicSolver_Test.solve_cubic_DISABLED
[       OK ] cubicSolver_Test.solve_cubic_DISABLED (3 ms)
[----------] 1 test from cubicSolver_Test (3 ms total)

* -vec-report0 is deprecated
* -diag-disable 1478 is not needed (has no effect)
* `Wcheck` (perform compile-time code checking for certain code) is
  changed to to `Wall` (enable warning and error diagnostics) as most
  warnings are due to upstream issues in fmtlib
* disable fast math optimization

Beyond, fix CYTHON_FALLTHROUGH for icc
@ischoegl
Copy link
Member Author

ischoegl commented Dec 8, 2021

@bryanwweber ... you had a point when you mentioned that icc is likely going to be used for a while, so I tried @tpg2114's suggestion to disable fast math even for that compiler. This indeed took care of tolerances, and I added the OneAPI classic runner back in.

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ischoegl !

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