-
Notifications
You must be signed in to change notification settings - Fork 37
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
Get code building with IBM XL on Power 9 #96
Conversation
bee568a
to
2b126a6
Compare
@Yurlungur @JoshuaSBrown @forrestglines Could one of you take a look at this? |
@AndrewGaspar if this is meant to be a temporary workaround till Catch2 gets fixed, could you please capture that in a comment in the file? That way future generations will not wonder why the lines are there in the first place. |
This is not a temporary workaround for Catch2 - these lines a necessary on their own for XL to work. Additionally, Catch2 needs an update. However, I discovered it’s literally only broken in the 2.11.3 release. |
OK, if that's the case, do you need to check more than one string for the compiler? Cmake appears to use either XL or XLCLANG for the XL compiler (CMakeFiles/3.16.2/CompilerIdC/CMakeCCompilerId.c). Do you need it for the XLCLANG compiler string as well? Perhaps MATCHES "^XL" instead of STEQUAL "XL" if this is the case. |
target_compile_features(parthenon PUBLIC cxx_std_14) | ||
if (CMAKE_CXX_COMPILER_ID STREQUAL "XL") | ||
target_compile_options(parthenon PUBLIC -std=c++1y -qxflag=disable__cplusplusOverride) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is "qxflag=disable__cplusplustOverride" needed, I'm having trouble finding what it is used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the note at the bottom of this page: https://www.ibm.com/support/knowledgecenter/SSXVZZ_16.1.1/com.ibm.xlcpp1611.lelinux.doc/language_ref/standard_features.html
I'm trying to compile this on Darwin, but CMake is complaining:
|
You configured like this:
But I think you meant this:
|
I didn't bother saying
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections from me once @jlippuner's issue is resolved.
This seems to work with just "XL", and I'm using the clang version of XL. They must have some compatibility mode in CMake? |
@jlippuner I should have specified the build process I followed better. You need to add this CXX flag to your CMake configuration:
You'll also want to load the gcc/9.3.0 module so that some of the .so files that are needed are found when you try to run the executable. I don't hard code this into the CMake because it's not portable. It's fully dependent on how the system admin chose to install XL. |
I wonder---can we get the power9 architecture integrated into the CI? @pgrete does your gitlab instance support that? |
If it works then that's fine. I was worried you'd get caught with this at some point that's all. |
At some point I thought we were going to try running the tests here on Darwin. @JoshuaSBrown - do you know what happened with that? (btw, why does GitHub only support threaded comments on line comments?!) |
Good question, you can just use quotes. I did not implement because 1. it was not a priority as the external guys already had a designated machine and 2. I think there are issues triggering the ci from a public github repo because darwin is an internal system. I can investigate this further if needed, but we might end up needing the enterprise version of github to make it work. |
Technically this is easily possible if we get access to a Power9 machine ("just" to install a gitlab-runner on there). |
I finally figured out the issue I was having on Darwin. The culprit is CMake. CMake version 3.12 thinks that the XL compiler does not support Should we increase the minimum CMake version required for Parthenon to 3.15? Here's what works for me on Darwin Power 9:
All tests pass for me without doing anything about Catch2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works now for me on Darwin if I use CMake >= 3.15
Perhaps we should document this configuration somewhere. I'd propose we have an extra doc file with build recipes for common machines we all use. It could save us all a lot of headache. I'll start an issue.
I lean towards keeping the cmake requirements down and just documenting this issue. If we require newer versions of cmake it can become a pain on, e.g., an Ubuntu LTS laptop. (Something I've already experienced with our build system.) |
Thanks for doing that! I think we can use Darwin, which already has a gitlab instance running on it. But I suspect there are some technical and administrative hurdles to overcome.... to, for example, make the CI output public. @gshipman and @JoshuaSBrown are probably the people to ask about that. Also @junghans. I'll start an issue where we can continue the discussion. |
PR Summary
This change gets the code building with IBM XL 16.1.1.7. Only required a couple small CMake changes.
The tests currently fail due to an issue with IBM XL when used with Catch2. When the fix to Catch2 is merged, I will update our Catch2 version: catchorg/Catch2#1907
PR Checklist