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.

Sign up for GitHub

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

Jump to bottom

[openmp] On Windows, fix standalone cmake build #80174

Merged
merged 3 commits into from
Feb 1, 2024
Merged

[openmp] On Windows, fix standalone cmake build #80174

merged 3 commits into from
Feb 1, 2024

Conversation

aganea
Copy link
Member

@aganea aganea commented Jan 31, 2024

This fixes: #80117

@aganea aganea added cmake Build system in general and CMake in particular openmp labels Jan 31, 2024
@llvmbot llvmbot added openmp:libomp OpenMP host runtime openmp:libomptarget OpenMP offload runtime labels Jan 31, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 31, 2024

@llvm/pr-subscribers-openmp

Author: Alexandre Ganea (aganea)

Changes

This fixes: #80117

Full diff: https://github.com/llvm/llvm-project/pull/80174.diff

1 Files Affected:

  • (modified) openmp/cmake/HandleOpenMPOptions.cmake (+2-2)
diff --git a/openmp/cmake/HandleOpenMPOptions.cmake b/openmp/cmake/HandleOpenMPOptions.cmake
index 201aeabbd3df9..79dfa334d749e 100644
--- a/openmp/cmake/HandleOpenMPOptions.cmake
+++ b/openmp/cmake/HandleOpenMPOptions.cmake
@@ -44,9 +44,9 @@ append_if(OPENMP_HAVE_DATA_SECTIONS "-fdata-sections" CMAKE_C_FLAGS CMAKE_CXX_FL
 
 if (MSVC)
   # Disable "warning C4201: nonstandard extension used: nameless struct/union"
-  append("-wd4201" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+  append_if(TRUE "-wd4201" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   
   # Disable "warning C4190: '__kmpc_atomic_cmplx8_rd' has C-linkage specified, but returns
   # UDT '__kmp_cmplx64_t' which is incompatible with C"
-  append("-wd4190" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+  append_if(TRUE "-wd4190" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 endif()

@aganea aganea mentioned this pull request Jan 31, 2024
Closed
aganea
aganea commented Jan 31, 2024
View reviewed changes
openmp/cmake/HandleOpenMPOptions.cmake Outdated
@@ -44,9 +44,9 @@ append_if(OPENMP_HAVE_DATA_SECTIONS "-fdata-sections" CMAKE_C_FLAGS CMAKE_CXX_FL

if (MSVC)
# Disable "warning C4201: nonstandard extension used: nameless struct/union"
append("-wd4201" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
append_if(TRUE "-wd4201" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
Copy link
Member Author

Choose a reason for hiding this comment

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

Using append_if(TRUE ...) because on MSVC silencing unknown warnings is a silent action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use list(APPEND ...) here?

Copy link
Member Author

Choose a reason for hiding this comment

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

string(APPEND ...) perhaps but it needs two lines so it's just shorter to write append_if(TRUE ..)

@shiltian
Copy link
Contributor

append_if(TRUE...) looks like a "hack" to me. Can we add the def of append and implement append_if with that?

@aganea
Copy link
Member Author

aganea commented Jan 31, 2024

As suggested.

shiltian
shiltian reviewed Jan 31, 2024
View reviewed changes
openmp/cmake/HandleOpenMPOptions.cmake
@@ -9,6 +9,14 @@ if (NOT COMMAND append_if)
endfunction()
Copy link
Contributor

Choose a reason for hiding this comment

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

Then here we can just call append

@h-vetinari
Copy link
Contributor

As suggested.

Tested with the status of this PR (4654c3d), and can confirm that windows build succeeds now. Thanks a lot!

@aganea aganea merged commit d2565bb into llvm:main Feb 1, 2024
4 checks passed
@aganea aganea deleted the fix_openmp_cmake branch February 1, 2024 13:14
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 1, 2024
@aganea @llvmbot
[openmp] On Windows, fix standalone cmake build (llvm#80174)
db86534 
This fixes: llvm#80117

(cherry picked from commit d2565bb)
smithp35 pushed a commit to smithp35/llvm-project that referenced this pull request Feb 1, 2024
@aganea @smithp35
[openmp] On Windows, fix standalone cmake build (llvm#80174)
718af44 
This fixes: llvm#80117
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this pull request Feb 1, 2024
@aganea
[openmp] On Windows, fix standalone cmake build (llvm#80174)
13daf9f 
This fixes: llvm#80117
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 2, 2024
@aganea @tstellar
[openmp] On Windows, fix standalone cmake build (llvm#80174)
1cfd46f 
This fixes: llvm#80117

(cherry picked from commit d2565bb)
agozillon pushed a commit to agozillon/llvm-project that referenced this pull request Feb 5, 2024
@aganea @agozillon
[openmp] On Windows, fix standalone cmake build (llvm#80174)
d6dbf02 
This fixes: llvm#80117
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
@aganea @tstellar
[openmp] On Windows, fix standalone cmake build (llvm#80174)
ea9e485 
This fixes: llvm#80117

(cherry picked from commit d2565bb)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
@aganea @tstellar
[openmp] On Windows, fix standalone cmake build (llvm#80174)
5e546e6 
This fixes: llvm#80117

(cherry picked from commit d2565bb)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
@aganea @tstellar
[openmp] On Windows, fix standalone cmake build (llvm#80174)
b01853d 
This fixes: llvm#80117

(cherry picked from commit d2565bb)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
@aganea @tstellar
[openmp] On Windows, fix standalone cmake build (llvm#80174)
450905d 
This fixes: llvm#80117

(cherry picked from commit d2565bb)
@pointhex pointhex mentioned this pull request May 7, 2024
Closed
@aemerson aemerson mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shiltian shiltian shiltian left review comments

@jhuber6 jhuber6 jhuber6 approved these changes

Assignees
No one assigned
Labels
cmake Build system in general and CMake in particular openmp:libomp OpenMP host runtime openmp:libomptarget OpenMP offload runtime openmp
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: openmp/cmake/HandleOpenMPOptions.cmake uses CMake function append without defining it
5 participants
@aganea @llvmbot @shiltian @h-vetinari @jhuber6