-
Notifications
You must be signed in to change notification settings - Fork 244
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
Support IntelLLVM compiler #2224
base: develop
Are you sure you want to change the base?
Support IntelLLVM compiler #2224
Conversation
Is this something for only Hercules, or will other systems be implementing IntelLLVM ? |
Eventually we must switch to IntelLLVM on all platforms. When, I don't know. But classic Intel compilers are now deprecated, see: NOTE: Intel® Fortran Compiler Classic (ifort) is now deprecated and will be discontinued in late 2024. Intel recommends that customers transition now to using the LLVM-based Intel® Fortran Compiler (ifx) for continued Windows* and Linux* support, new language support, new language features, and optimizations. For more information on ifx, see the Intel® Fortran Compiler Developer Guide and Reference and the Porting Guide for ifort Users to ifx. |
Compilation with Intel LLVM on Hera fails with this error:
|
All coupled debug tests fail on Hercules with this error:
This is looks like a bug in the ifx. |
@NickSzapiro-NOAA Would you please take a look at what is happening here w/ CICE? You'll need to get from @DusanJovic-NOAA build instructions and maybe a sandbox run directory. |
Of course, @DeniseWorthen @DusanJovic-NOAA. Please let me know if there are any special instructions needed to reproduce. On first pass, it's curious that the ice speed may be the first call to accum_hist with an array that was not allocated: |
I was able to create a small test program that reproduces an error identical to the one we see in CICE. Dom (@climbfuji) confirmed that it works with the latest Intel LLVM compiler (ifx (IFX) 2024.1.0 20240308), so it is probably just a bug in the older versions. Version on Hercules is (ifx (IFX) 2023.1.0 20230320). On Hera it's even older (ifx (IFORT) 2022.0.0 20211123), and that version can not even compile the code with MOM6. See above error. We need to find a machine (NOAA R&D) on which we can get the latest version of Intel LLVM compiler (2024.1.0) installed. |
I can install it on Hercules in the EPIC spack-stack space, but we still need to manage building all dependencies with it. |
If it is confirmed that the latest Intel LLVM compiler fixes the issue, let's ask sysadmin to install the latest version. |
Eventually yes, but for now I think we can still build all libraries (spack-stack) with ifort (but the new version 2024) and only the model with ifx. Object files and module files should be binary compatible between ifort and ifx. According to Intel: |
Except that there's no more icc anc icpc, so at least for those we have to switch to icx and icpx. It should be fairly straightforward to build just the dependencies for the ufs-weather-model with ifort and/or ifx and get all the other packages compiled in a second step. |
….conf for testing
@DusanJovic-NOAA Does filling worka first (https://github.com/NOAA-EMC/CICE/blob/emc/develop/cicecore/cicedyn/analysis/ice_history.F90#L2321-L2331) give a temporary fix? |
I think it should. Looks like having an array expression (that depends on iblk) as an actual subroutine argument is what fails. |
Currently debug builds fail (on Hera) with the following error:
That's why all debug tests are currently disabled in All non-debug tests successfully compile, execute and reproduce it's own outputs, ie:
is successful. |
Regression test passed on Hera |
@DusanJovic-NOAA We can commit this pr today. Please, go ahead to sync up branch. |
@jkbk2004 We would to update CICE submodule to use an updated PR and branch that includes both LLVM updates and some other updates from upstream. Results are expected to still be b4b. |
|
CICE submodule has been updated. Ready for regression testing. |
@DusanJovic-NOAA We've been asked to add someone to our CODEOWNERS file in the ccpp-physics repo and we're trying to find a convenient PR to do this in so that we don't open a separate PR for this small task. Of course, it doesn't affect any testing, but you would need to merge a PR into ufs-community/ccpp-physics#220 and update submodule hashes. Would you be willing to do this? If not, no big deal. If yes, I'll prepare a PR into your branch for ufs-community/ccpp-physics#220. |
Sure. I can do that. Please open PR to my ccpp-physics branch. @jkbk2004 Please hold on for few more minutes with starting RT. |
@DusanJovic-NOAA Thanks. See DusanJovic-NOAA/ccpp-physics#1 |
Merged ccpp-physics PR, and updated submodules. Please double check and if everything looks good I think we can finally start RT |
@DusanJovic-NOAA Thanks for granting this last-minute request! |
The CICE #ifndef LLVM PR was merged at CICE-Consortium. Here is a new EMC/CICE branch (https://github.com/NickSzapiro-NOAA/CICE/tree/cice_llvm) and PR (NOAA-EMC/CICE#93) for use in this PR. It is the same lines of code for us in CICE and cleaner git history. If can use, there is no need to repeat testing. |
Commit Queue Requirements:
Description:
Add preliminary support for Intel LLVM compiler. Added new top-level
cmake/IntelLLVM.cmake
configuration file. Update CMakeLists.txt in various submodules to recognize IntelLLVM as supported compiler and add Intel LLVM compiler flags.Useful porting guide: https://www.intel.com/content/www/us/en/developer/articles/guide/porting-guide-for-ifort-to-ifx.html
At this moment I only tried to build the model on Hercules.
In order to use new compiler you need to set the following three environment variables:
export I_MPI_CC=icx
export I_MPI_CXX=icpx
export I_MPI_F90=ifx
Commit Message:
Priority:
Git Tracking
UFSWM:
Sub component Pull Requests:
UFSWM Blocking Dependencies:
Changes
Regression Test Changes (Please commit test_changes.list):
Input data Changes:
Library Changes/Upgrades:
Testing Log: