-
Notifications
You must be signed in to change notification settings - Fork 138
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
Update CMake files to support windows #464
Conversation
fa158dc
to
6854c2b
Compare
@sscalpone, @gklimowicz, this PR is ready for review. This does not add support for Windows, but adds a few miscellaneous changes to the CMake files that can be reviewed independently. (I figured small PRs are easier to review.) |
@isuruf Were the Pull requests for Windows finally merged into flang or they are still not merged like this one? What is the status of current flang master for Windows and what will be the status if we merge all the Windows patches? |
Only a couple were merged. See https://github.com/flang-compiler/flang/pulls?q=is%3Apr+author%3Aisuruf+ I haven't checked in a long time, but the pull request #288 was the original PR and I broke it into smaller PRs, but not all of the changes of #288 were broken into smaller PRs. After the other PRs are done, will have to check what needs to be ported over from #288 |
I meant #288 above. Edited the comment |
HI @isuruf, I'm interested in Windows support of flang. What's your plan with it? Do you want to merge these patch set? |
@isuruf We now have commit access and PRs are being merged as you might have noticed. We can process this and other Windows PRs if they are ready and if there is interest from others. There is also a biweekly call discussing PRs and Classic Flang in general. Minutes and teams meeting link can be found in the google doc below. |
@kiranchandramohan, that'd be great. This PR is ready and so are the other PRs that I have authored (https://github.com/flang-compiler/flang/pulls/isuruf). Merging all of those would be the first step towards Windows support. There's more work to do after these PRs are merged. |
Can you rebase these PRs and also give the order in which these should be tested? |
@isuruf the github action CI is enabled now, it creates a build on X86 linux and does make check-all. Would you be able to check and edit your PR so that the CI passes? |
Yes, will do. I'm talking with CMake developers about how to fix this in https://gitlab.kitware.com/cmake/cmake/-/issues/21463 |
Hello, |
@isuruf Hi, What about your CMake fix? I saw you already have a WIP patch for that (at the moment the PR is closed), are you going to merge this fix? |
Hello All, |
@seshakalyur We are in the preliminary stages of Windows support. The first task is to enable CMake support using patches from @isuruf so that we can start compiling the source of the compiler. Then we will look at the source compilation issues. I believe @kaadam has taken this CMake changes and probably others and got the CMake part working. |
@isuruf @kiranchandramohan What about if intrinsic keywords are removed as a workaround from ieee_arithmetic.F95 and ieee_exceptions.F95? |
@isuruf ping? |
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.
LGTM, (I will close #1003 )
Anything else to do here? |
@bryanpkc do you have any other comment on this PR? Could you look at it please? Thanks in advance. |
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.
I don't have any more comment on this PR, except that the commits should be squashed and a comprehensive commit message should be written for the squashed commit.
- Set the target linker language for the flang libraries to CXX on windows. This makes sure link.exe from Visual Studio is used to link the libraries, even though the libraries include fortran code. CMake is not able to detect this since the flang fortran compiler is not working at CMake configure time and thus the CMAKE_Fortran_COMPILER_ID is unknown. - Add WINDOWS_EXPORT_ALL_SYMBOLS for shared flang library - Use "lib" prefix instead of "_static" suffix for static runtime libraries. This is more in sync with windows default names (e.g. ucrt.lib as import library for ucrt.dll and libucrt.lib for the static runtime). - Set WINDOWS_EXPORT_ALL_SYMBOLS property for for shared flangrti and ompstub targets. This works around the issue with missing __declspec(dllexport/dllimport) declerations in the source files.
- Call project command at the beginning if compiling flang in standalone mode This makes sure that CMake variables like CMAKE_SYSTEM_PROCESSOR are defined and removes the need for hacks like calling uname -m - Prevent CMake from running tests to check if flang works flang.exe is not fully functional and CMake tries to run checks on the Fortran compiler (more than in the Unix case) and fails. By setting CMake options like CMAKE_Fortran_COMPILER_SUPPORTS_F90, CMake accepts flang.exe as the Fortran compiler - Alias AMD64/amd64 to x86_64 - Alias arm64 to x86_64 - Preprocessor options like TARGET_LINUX are replaced with TARGET_${OS} - Set Fortran_MODULE_DIRECTORY to a different directory for static builds in Ninja generator This makes it possible for flang to be built with Ninja on windows. For other generators https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5518 needs to be used when it is in a CMake release - Link to libm only on non-windows - Remove .so from name when linking When linking libomp.so is not portable. Use NAMES omp libomp for portability - Call built executables using the target name instead of full path The full path to the built executable given is not portable, but using the target name is portable as CMake will internally figure out the full path - Sort upperilm.in using portable CMake code
I squashed the commits into two commits. One for all the commits authored by me and one for all the commits by Albert. |
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.
LGTM, although I'd feel better if an Windows expert could also approve that.
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.
LGTM.
I've picked some changes from #288 that can be merged easily.