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

Update CMake files to support Windows platform #1003

Closed
wants to merge 1 commit into from

Conversation

kaadam
Copy link
Collaborator

@kaadam kaadam commented Feb 24, 2021

This PR aims to enable the build for Windows x64. Windows port of Flang is not ready yet, but it would be nice if anybody could run a build and check the current status.
This PR is created based on #464, since there is no any activity on that that's why I opened a new one.

@xoviat
Copy link
Collaborator

xoviat commented Feb 24, 2021

Note: I had planned to submit a PR to update the CMake files after the current PRs are merged. I'd like to wait until the runtime and pgmath PRs are merged, then I can see where we are.

@isuruf
Copy link
Collaborator

isuruf commented Feb 24, 2021

On windows if you are building with clang with MSVC ABI MSVC is still true. the only case where WIN32 is true and MSVC is not is if you are using MinGW/MSYS. In that case, the libraries are more like unix with libm also available.

@kaadam
Copy link
Collaborator Author

kaadam commented Feb 24, 2021

On windows if you are building with clang with MSVC ABI MSVC is still true. the only case where WIN32 is true and MSVC is not is if you are using MinGW/MSYS. In that case, the libraries are more like unix with libm also available.

Yes, you're right. I tested with clang for windows. It seems It was a side effect of my built clang (I don't know why). I dropped that change. Thanks for the comment.

@kaadam
Copy link
Collaborator Author

kaadam commented Feb 24, 2021

Note: I had planned to submit a PR to update the CMake files after the current PRs are merged. I'd like to wait until the runtime and pgmath PRs are merged, then I can see where we are.

@xoviat sorry, I think it would be nice to enable the basic build, maybe it makes the review process easier. In my opinion it could be merge independently of the other Windows changes, of course if you and others agree with that.

@@ -9,45 +9,52 @@ cmake_minimum_required(VERSION 3.3)
# In order to bootstrap the runtime library we need to skip
# CMake's Fortran tests
SET(CMAKE_Fortran_COMPILER_WORKS 1)
SET(CMAKE_Fortran_ABI_COMPILED 0)
SET(CMAKE_Fortran_COMPILER_SUPPORTS_F90 1)
SET(CMAKE_Fortran_MODDIR_FLAG "-J ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you run CMake with -DCMAKE_Fortran_COMPILER_ID=Flang? That should set CMAKE_Fortran_MODDIR_FLAG correctly.

Fortran_MODULE_DIRECTORY ${CMAKE_BINARY_DIR}/include-static
)
if (MSVC)
set_property(TARGET flang_static PROPERTY OUTPUT_NAME libflang)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please indent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Line 489 as well.

@isuruf
Copy link
Collaborator

isuruf commented Jun 22, 2021

@bryanpkc, can you review #464 as that PR has the commits with authors correctly?

@@ -1,3 +1,4 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Spurious blank line.

@@ -7,11 +7,18 @@
set(OMPSTUB_SRC init_nomp.c ompstubs.c)

add_flang_library(ompstub_static ${OMPSTUB_SRC})
if (MSVC)
set_property(TARGET ompstub_static PROPERTY OUTPUT_NAME libompstub)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please indent this line and line 13.

@kaadam kaadam closed this Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants