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

[CMAKE][AMDGPU] fix build failure caused by PR #133619 #133776

Closed
wants to merge 1 commit into from
Closed

[CMAKE][AMDGPU] fix build failure caused by PR #133619 #133776

wants to merge 1 commit into from

Conversation

farzonl
Copy link
Member

@farzonl farzonl commented Mar 31, 2025

While clangCodeGenTargetBuiltins and clangCodeGenTargets are static libraries clangCodeGen is not and so this is creating a circular reference in the linux amdgpu-offload CIs on ubuntu and RHEL.

removing the circular reference doesn't break anything and looks to be a vestigial element of the origional PR.

@farzonl
[CMAKE][AMDGPU] fix build failure caused by PR llvm#133619
a46aa12 
While clangCodeGenTargetBuiltins and clangCodeGenTargets are static
libraries clangCodeGen is not and so this is creating a circular
reference in the linux amdgpu-offload CIs on ubuntu and RHEL.

removing the circular reference doesn't break anything and looks to
be a vestigial element of the origional PR.
@farzonl farzonl added the cmake Build system in general and CMake in particular label Mar 31, 2025
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Mar 31, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 31, 2025 •
edited
Loading

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-clang

Author: Farzon Lotfi (farzonl)

Changes

While clangCodeGenTargetBuiltins and clangCodeGenTargets are static libraries clangCodeGen is not and so this is creating a circular reference in the linux amdgpu-offload CIs on ubuntu and RHEL.

removing the circular reference doesn't break anything and looks to be a vestigial element of the origional PR.

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

2 Files Affected:

  • (modified) clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt (-5)
  • (modified) clang/lib/CodeGen/Targets/CMakeLists.txt (-5)
diff --git a/clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt b/clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt
index 8526c063b4593..76be68a11d02a 100644
--- a/clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt
+++ b/clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt
@@ -12,8 +12,3 @@ add_clang_library(clangCodeGenTargetBuiltins STATIC
   WebAssembly.cpp
   X86.cpp
 )
-
-target_link_libraries(clangCodeGenTargetBuiltins
-  PRIVATE
-  clangCodeGen
-)
diff --git a/clang/lib/CodeGen/Targets/CMakeLists.txt b/clang/lib/CodeGen/Targets/CMakeLists.txt
index fd79b6191b379..6b6e7ce5b0b29 100644
--- a/clang/lib/CodeGen/Targets/CMakeLists.txt
+++ b/clang/lib/CodeGen/Targets/CMakeLists.txt
@@ -28,8 +28,3 @@ add_clang_library(clangCodeGenTargets STATIC
   X86.cpp
   XCore.cpp
 )
-
-target_link_libraries(clangCodeGenTargets
-  PRIVATE
-  clangCodeGen
-)

@llvmbot
Copy link
Member

llvmbot commented Mar 31, 2025

@llvm/pr-subscribers-clang-codegen

Author: Farzon Lotfi (farzonl)

Changes

While clangCodeGenTargetBuiltins and clangCodeGenTargets are static libraries clangCodeGen is not and so this is creating a circular reference in the linux amdgpu-offload CIs on ubuntu and RHEL.

removing the circular reference doesn't break anything and looks to be a vestigial element of the origional PR.

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

2 Files Affected:

  • (modified) clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt (-5)
  • (modified) clang/lib/CodeGen/Targets/CMakeLists.txt (-5)
diff --git a/clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt b/clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt
index 8526c063b4593..76be68a11d02a 100644
--- a/clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt
+++ b/clang/lib/CodeGen/TargetBuiltins/CMakeLists.txt
@@ -12,8 +12,3 @@ add_clang_library(clangCodeGenTargetBuiltins STATIC
   WebAssembly.cpp
   X86.cpp
 )
-
-target_link_libraries(clangCodeGenTargetBuiltins
-  PRIVATE
-  clangCodeGen
-)
diff --git a/clang/lib/CodeGen/Targets/CMakeLists.txt b/clang/lib/CodeGen/Targets/CMakeLists.txt
index fd79b6191b379..6b6e7ce5b0b29 100644
--- a/clang/lib/CodeGen/Targets/CMakeLists.txt
+++ b/clang/lib/CodeGen/Targets/CMakeLists.txt
@@ -28,8 +28,3 @@ add_clang_library(clangCodeGenTargets STATIC
   X86.cpp
   XCore.cpp
 )
-
-target_link_libraries(clangCodeGenTargets
-  PRIVATE
-  clangCodeGen
-)

@farzonl farzonl mentioned this pull request Mar 31, 2025
Merged
asudarsa
asudarsa approved these changes Mar 31, 2025
View reviewed changes
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM

asudarsa
asudarsa reviewed Mar 31, 2025
View reviewed changes
Copy link
Contributor

@asudarsa asudarsa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I tried to build it locally and I got a series of 'undefined reference' errors. Sorry, I will need to revert my approval.
Attn. @jhuber6

Thanks

@vzakhari
Copy link
Contributor

@farzonl can you please revert the original commit? It seems to break just any LLVM build, so I guess the CI testing of all new PRs should be affected.

@farzonl
Copy link
Member Author

farzonl commented Mar 31, 2025

@farzonl can you please revert the original commit? It seems to break just any LLVM build, so I guess the CI testing of all new PRs should be affected.

I don't see it breaking any build in LLVM, just the amd offload ones. Al the Premerge tests also passed. I suspect the AMDGPUBot.cmake is doing something unique. Will study it more

but the PR to revert is here #133795

@farzonl farzonl closed this Mar 31, 2025
@farzonl
Copy link
Member Author

farzonl commented Mar 31, 2025

I tried to build it locally and I got a series of 'undefined reference' errors. Sorry, I will need to revert my approval. Attn. @jhuber6

Thanks

Could you provide the failures you were seeing, specific build instructions, and platform you were doing this on.

@vzakhari
Copy link
Contributor

@farzonl can you please revert the original commit? It seems to break just any LLVM build, so I guess the CI testing of all new PRs should be affected.

I don't see it breaking any build in LLVM, just the amd offload ones. Al the Premerge tests also passed. I suspect the AMDGPUBot.cmake is doing something unique. Will study it more

but the PR to revert is here #133795

Thanks! I think, it only affects the -DBUILD_SHARED_LIBS=ON builds, so, indeed, not all the builds :)

A couple more examples: https://lab.llvm.org/buildbot/#/builders/145/builds/6156 https://lab.llvm.org/buildbot/#/builders/89/builds/19738

@asudarsa
Copy link
Contributor

asudarsa commented Mar 31, 2025 •
edited
Loading

@farzonl can you please revert the original commit? It seems to break just any LLVM build, so I guess the CI testing of all new PRs should be affected.

I don't see it breaking any build in LLVM, just the amd offload ones. Al the Premerge tests also passed. I suspect the AMDGPUBot.cmake is doing something unique. Will study it more
but the PR to revert is here #133795

Thanks! I think, it only affects the -DBUILD_SHARED_LIBS=ON builds, so, indeed, not all the builds :)

A couple more examples: https://lab.llvm.org/buildbot/#/builders/145/builds/6156 https://lab.llvm.org/buildbot/#/builders/89/builds/19738

Thanks @vzakhari, I also am building with -DBUILD_SHARED_LIBS=ON
An example error:
/usr/bin/ld: tools/clang/lib/CodeGen/Targets/CMakeFiles/obj.clangCodeGenTargets.dir/WebAssembly.cpp.o:(.data.rel.ro._ZTV18WebAssemblyABIInfo[_ZTV18WebAssemblyABIInfo]+0x48): undefined reference to `clang::CodeGen::ABIInfo::isHomogeneousAggregateSmallEnough(clang::Type const*, unsigned long) const'

@farzonl farzonl mentioned this pull request Apr 1, 2025
Merged
farzonl added a commit that referenced this pull request Apr 7, 2025
@farzonl
Reland [Clang][Cmake] fix libtool duplicate member name warnings (#13…
0d71d9a 
…3850)

fixes #133199

As of the third commit the fix to the linker missing references in
`Targets/DirectX.cpp` found in
#133776 was fixed by moving
`HLSLBufferLayoutBuilder.cpp` to `clang/lib/CodeGen/Targets/`.

It fixes the circular reference issue found in
#133619 for all
`-DBUILD_SHARED_LIBS=ON` builds by removing `target_link_libraries` from
the sub directory cmake files.

testing for amdgpu offload was done via
`cmake -B ../llvm_amdgpu -S llvm -GNinja -C
offload/cmake/caches/Offload.cmake -DCMAKE_BUILD_TYPE=Release`

PR #132252 Created a second
file that shared <TargetName>.cpp in clang/lib/CodeGen/CMakeLists.txt

For example There were two AMDGPU.cpp's one in TargetBuiltins and the
other in Targets. Even though these were in different directories
libtool warns that it might not distinguish them because they share the
same base name.

There are two potential fixes. The easy fix is to rename one of them and
keep one cmake file. That solution though doesn't future proof this
problem in the event of a third <TargetName>.cpp and it seems teams want
to just use the target name

#132252 (comment).

The alternative fix that this PR went with is to seperate the cmake
files into their own sub directories as static libs.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Apr 7, 2025
@farzonl @github-actions
Automerge: Reland [Clang][Cmake] fix libtool duplicate member name wa…
5fa816d 
…rnings (#133850)

fixes llvm/llvm-project#133199

As of the third commit the fix to the linker missing references in
`Targets/DirectX.cpp` found in
llvm/llvm-project#133776 was fixed by moving
`HLSLBufferLayoutBuilder.cpp` to `clang/lib/CodeGen/Targets/`.

It fixes the circular reference issue found in
llvm/llvm-project#133619 for all
`-DBUILD_SHARED_LIBS=ON` builds by removing `target_link_libraries` from
the sub directory cmake files.

testing for amdgpu offload was done via
`cmake -B ../llvm_amdgpu -S llvm -GNinja -C
offload/cmake/caches/Offload.cmake -DCMAKE_BUILD_TYPE=Release`

PR llvm/llvm-project#132252 Created a second
file that shared <TargetName>.cpp in clang/lib/CodeGen/CMakeLists.txt

For example There were two AMDGPU.cpp's one in TargetBuiltins and the
other in Targets. Even though these were in different directories
libtool warns that it might not distinguish them because they share the
same base name.

There are two potential fixes. The easy fix is to rename one of them and
keep one cmake file. That solution though doesn't future proof this
problem in the event of a third <TargetName>.cpp and it seems teams want
to just use the target name

llvm/llvm-project#132252 (comment).

The alternative fix that this PR went with is to seperate the cmake
files into their own sub directories as static libs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asudarsa asudarsa

@jhuber6 jhuber6

Assignees
No one assigned
Labels
backend:AMDGPU clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category cmake Build system in general and CMake in particular
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@farzonl @llvmbot @vzakhari @asudarsa @jhuber6