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

fix linking on riscv64 #74167

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix linking on riscv64 #74167

wants to merge 2 commits into from

Conversation

vpalatin
Copy link

@vpalatin vpalatin commented Dec 2, 2023

As libatomic is not included in libgcc_s, when building on riscv64
(on Ubuntu 22.04) using rtlib set to libgcc and gnu ld as the linker,
the linking of a few utilities fails on some __atomic_X symbols provided by libatomic.

For dsymutils, actually reviews.llvm.org/D137799 has commented out the line potentially adding a
-latomic when linking by using the LLVM_ATOMIC_LIB variable without providing any reason. Revert this piece of the change.

For several libMLIR consumers, add the LLVM_ATOMIC_LIB variable where needed.

As libatomic is not included in libgcc_s, when building on riscv64
(on Ubuntu 22.04) using rtlib set to libgcc and gnu ld as the linker,
the linking of dsymutil fails on some __atomic_X symbols provided by libatomic.

reviews.llvm.org/D137799 has commented out the line potentially adding a
`-latomic` when linking by using the LLVM_ATOMIC_LIB variable without
providing any reason. Revert this piece of the change.
As libatomic is not included in libgcc_s, when building on riscv64
(on Ubuntu 22.04) using rtlib set to libgcc and gnu ld as the linker,
the linking of several libMLIR consumers fails on some __atomic_X
symbols provided by libatomic.

Add the LLVM_ATOMIC_LIB variable potentially adding a `-latomic` when linking.
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure debuginfo mlir labels Dec 2, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 2, 2023

@llvm/pr-subscribers-mlir
@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-debuginfo

Author: Vincent Palatin (vpalatin)

Changes

As libatomic is not included in libgcc_s, when building on riscv64
(on Ubuntu 22.04) using rtlib set to libgcc and gnu ld as the linker,
the linking of a few utilities fails on some __atomic_X symbols provided by libatomic.

For dsymutils, actually reviews.llvm.org/D137799 has commented out the line potentially adding a
-latomic when linking by using the LLVM_ATOMIC_LIB variable without providing any reason. Revert this piece of the change.

For several libMLIR consumers, add the LLVM_ATOMIC_LIB variable where needed.


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

4 Files Affected:

  • (modified) llvm/tools/dsymutil/CMakeLists.txt (+1-1)
  • (modified) mlir/lib/Pass/CMakeLists.txt (+2)
  • (modified) mlir/lib/Tools/lsp-server-support/CMakeLists.txt (+2)
  • (modified) mlir/tools/mlir-shlib/CMakeLists.txt (+2)
diff --git a/llvm/tools/dsymutil/CMakeLists.txt b/llvm/tools/dsymutil/CMakeLists.txt
index c612bfd9150c47f..de1da71a64dc124 100644
--- a/llvm/tools/dsymutil/CMakeLists.txt
+++ b/llvm/tools/dsymutil/CMakeLists.txt
@@ -44,4 +44,4 @@ if(APPLE AND NOT LLVM_TOOL_LLVM_DRIVER_BUILD)
   target_link_libraries(dsymutil PRIVATE "-framework CoreFoundation")
 endif(APPLE AND NOT LLVM_TOOL_LLVM_DRIVER_BUILD)
 
-# target_link_libraries(dsymutil PRIVATE ${LLVM_ATOMIC_LIB})
+target_link_libraries(dsymutil PRIVATE ${LLVM_ATOMIC_LIB})
diff --git a/mlir/lib/Pass/CMakeLists.txt b/mlir/lib/Pass/CMakeLists.txt
index 5ca9b4163bb8a8f..d2628aaa7274ec4 100644
--- a/mlir/lib/Pass/CMakeLists.txt
+++ b/mlir/lib/Pass/CMakeLists.txt
@@ -16,4 +16,6 @@ add_mlir_library(MLIRPass
   LINK_LIBS PUBLIC
   MLIRAnalysis
   MLIRIR
+  PRIVATE
+  ${LLVM_ATOMIC_LIB}
   )
diff --git a/mlir/lib/Tools/lsp-server-support/CMakeLists.txt b/mlir/lib/Tools/lsp-server-support/CMakeLists.txt
index 48a96016b792fce..8f7e26340067f1e 100644
--- a/mlir/lib/Tools/lsp-server-support/CMakeLists.txt
+++ b/mlir/lib/Tools/lsp-server-support/CMakeLists.txt
@@ -10,4 +10,6 @@ add_mlir_library(MLIRLspServerSupportLib
 
   LINK_LIBS PUBLIC
   MLIRSupport
+  PRIVATE
+  ${LLVM_ATOMIC_LIB}
   )
diff --git a/mlir/tools/mlir-shlib/CMakeLists.txt b/mlir/tools/mlir-shlib/CMakeLists.txt
index 32fe833cee4ea78..59e0f7078288355 100644
--- a/mlir/tools/mlir-shlib/CMakeLists.txt
+++ b/mlir/tools/mlir-shlib/CMakeLists.txt
@@ -39,6 +39,8 @@ if(LLVM_BUILD_LLVM_DYLIB)
     ${_OBJECTS}
     LINK_LIBS
     ${_DEPS}
+    PRIVATE
+    ${LLVM_ATOMIC_LIB}
 
     LINK_COMPONENTS
     ${mlir_llvm_link_components}

@vpalatin
Copy link
Author

vpalatin commented Dec 2, 2023

@abrachet
The line I'm uncommenting was originally commented out by your change https://reviews.llvm.org/D137799
maybe you have something to say about the rationale

@brad0
Copy link
Contributor

brad0 commented Dec 2, 2023

@abrachet The line I'm uncommenting was originally commented out by your change https://reviews.llvm.org/D137799 maybe you have something to say about the rationale

Looking at the overall diff it looks like that might have been an unrelated change that snuck in and no one noticed.

@@ -16,4 +16,6 @@ add_mlir_library(MLIRPass
LINK_LIBS PUBLIC
MLIRAnalysis
MLIRIR
PRIVATE
${LLVM_ATOMIC_LIB}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why isn't this part of add_mlir_library? Seems a bit random to add this here and there right now.

Copy link
Author

Choose a reason for hiding this comment

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

fair point
I just put it in the specific places where it was needed (ie using the libatomic symbols) the same way it was done in other places in llvm (mlir never had support for this case).
but this will require some 'manual' maintenance as it is not obvious where these functions are used.
adding it globally to the helper might be a better avenue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debuginfo mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants