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] Link against libm on OpenBSD #70614

Merged
merged 1 commit into from
Nov 12, 2023
Merged

[OpenMP] Link against libm on OpenBSD #70614

merged 1 commit into from
Nov 12, 2023

Conversation

brad0
Copy link
Contributor

@brad0 brad0 commented Oct 30, 2023 •
edited

Needed for some math functions in libomp.

@llvmbot llvmbot added the openmp:libomp OpenMP host runtime label Oct 30, 2023
arsenm
arsenm reviewed Oct 30, 2023
View reviewed changes
openmp/runtime/cmake/LibompHandleFlags.cmake
@@ -140,7 +140,7 @@ function(libomp_get_libflags libflags)
if(LIBOMP_HAVE_SHM_OPEN_WITH_LRT)
libomp_append(libflags_local -lrt)
endif()
if(${CMAKE_SYSTEM_NAME} MATCHES "DragonFly|FreeBSD")
Copy link
Contributor

Choose a reason for hiding this comment

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

OS lists like this are bad, can you use check_library_exists

Copy link
Contributor

Choose a reason for hiding this comment

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

I think check_library_exists is not for the purpose of this case. For example, libm exists on a lot of systems, but we only want to link against it with proper linker flag when OS is BSD.

Copy link
Contributor

Choose a reason for hiding this comment

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

but why, you can do any kind of try_compile test

@brad0
Copy link
Contributor Author

brad0 commented Nov 3, 2023 •
edited

@shiltian So can I commit this?

@brad0
Copy link
Contributor Author

brad0 commented Nov 10, 2023

Ping.

shiltian
shiltian approved these changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@shiltian shiltian left a comment

Choose a reason for hiding this comment

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

The changes look good for now. We definitely want to try to revise it later based on what @arsenm suggested.

@brad0
Copy link
Contributor Author

brad0 commented Nov 12, 2023

The changes look good for now. We definitely want to try to revise it later based on what @arsenm suggested.

Oh for sure. Even if you did want to use something like check_library_exists or check_symbol_exists and do something similar to how librt is handled the issue is how to test and know when to use no-as-needed / as-needed.

@brad0 brad0 merged commit 5feebdc into llvm:main Nov 12, 2023
4 checks passed
@brad0 brad0 deleted the openmp_cmake_openbsd branch November 12, 2023 01:37
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
@brad0 @zahiraam
[OpenMP] Link against libm on OpenBSD (llvm#70614)
2cf55f7 
Needed for some math functions in libomp.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arsenm arsenm arsenm left review comments

@shiltian shiltian shiltian approved these changes

Assignees
No one assigned
Labels
openmp:libomp OpenMP host runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@brad0 @arsenm @shiltian @llvmbot