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

[Tool] Incorporate the LLVM third-party library #28383

Merged
merged 12 commits into from
Aug 25, 2023
Merged

[Tool] Incorporate the LLVM third-party library #28383

merged 12 commits into from
Aug 25, 2023

Conversation

Moonm3n
Copy link
Contributor

@Moonm3n Moonm3n commented Aug 1, 2023 •
edited
Loading

Related to #28331 , Adding the LLVM library to StarRocks.

The following impacts will occur after adding the LLVM third-party library:
1. The Docker image size will increase by 200MB, reaching 2.8GB.
2. The binary size of starrocks_be will increase by 47MB.
3. The time taken for a Third-Party Update will increase by roughly 10 minutes, reaching a total of 50 minutes.
4. The impact on the compilation time is little.

Additional: In the past, third-party libraries did not end with .tar.xz . It appears that the starrocks/dev-env-ubuntu image does not include xz-utils. Therefore, it may be necessary to install this utility using the command apt install xz-utils .

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr will affect users' behaviors
  • This pr needs user documentation (for new or modified features or behaviors)
  • I have added documentation for my new feature or new function

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.1
    • 3.0
    • 2.5
    • 2.4

@Moonm3n Moonm3n mentioned this pull request Aug 1, 2023
Closed
@mergify mergify bot assigned Moonm3n Aug 1, 2023
stdpain
stdpain reviewed Aug 1, 2023
View reviewed changes
be/CMakeLists.txt Outdated Show resolved Hide resolved
kevincai
kevincai reviewed Aug 2, 2023
View reviewed changes
be/CMakeLists.txt Show resolved Hide resolved
thirdparty/build-thirdparty.sh Outdated Show resolved Hide resolved
thirdparty/build-thirdparty.sh Show resolved Hide resolved
thirdparty/vars.sh Show resolved Hide resolved
@Moonm3n
Copy link
Contributor Author

Moonm3n commented Aug 5, 2023

Reason for not using precompiled packages:
LLVM does not provide official precompiled packages; all precompiled packages are contributions from developers.

Refer to:
llvm/llvm-project#60201
llvm/llvm-project#53892

@Moonm3n Moonm3n force-pushed the llvm_third_party branch 4 times, most recently from 9a3dbb7 to 95ca2b4 Compare August 7, 2023 04:03
@Moonm3n Moonm3n force-pushed the llvm_third_party branch 4 times, most recently from 1fb3574 to 4797cd8 Compare August 15, 2023 13:40
kevincai
kevincai reviewed Aug 16, 2023
View reviewed changes
be/CMakeLists.txt Outdated Show resolved Hide resolved
@kevincai
Copy link
Contributor

we may completely disable arm support of this llvm jit feature. don't bother support arm platform.

@kevincai
Copy link
Contributor

can also completely strip llvm related library debug symbol, we are not likely to debug into llvm related library, its debug symbol is useless.

@Moonm3n
Copy link
Contributor Author

Moonm3n commented Aug 18, 2023

can also completely strip llvm related library debug symbol, we are not likely to debug into llvm related library, its debug symbol is useless.

I'm slightly confused about this. When compiling LLVM, I used -DCMAKE_BUILD_TYPE="RELEASE". Should I use additional steps to strip the debug symbols?

@kevincai
Copy link
Contributor

  1. The time taken for a Third-Party Update will increase by roughly 20 minutes, reaching a total of 60 minutes.

can also completely strip llvm related library debug symbol, we are not likely to debug into llvm related library, its debug symbol is useless.

I'm slightly confused about this. When compiling LLVM, I used -DCMAKE_BUILD_TYPE="RELEASE". Should I use additional steps to strip the debug symbols?

You can use nm to check if the built artifacts contains any debug symbol. If all the deubgsymbols are removed, the final size of starrocks_be.debuginfo shouldn't increase as large as 1GB.

@Moonm3n
Copy link
Contributor Author

Moonm3n commented Aug 19, 2023

  1. The time taken for a Third-Party Update will increase by roughly 20 minutes, reaching a total of 60 minutes.

can also completely strip llvm related library debug symbol, we are not likely to debug into llvm related library, its debug symbol is useless.

I'm slightly confused about this. When compiling LLVM, I used -DCMAKE_BUILD_TYPE="RELEASE". Should I use additional steps to strip the debug symbols?

You can use nm to check if the built artifacts contains any debug symbol. If all the deubgsymbols are removed, the final size of starrocks_be.debuginfo shouldn't increase as large as 1GB.

It appears that the global CXXFLAGS have impacted the compilation behavior. I will fix this.

@wanpengfei-git
Copy link
Collaborator

[FE PR Coverage Check]

😍 pass : 0 / 0 (0%)

@kevincai
Copy link
Contributor

#29813 PR added xz-utils into ubuntu env, please rebase this PR to latest main HEAD.

kevincai
kevincai reviewed Aug 24, 2023
View reviewed changes
thirdparty/build-thirdparty.sh Show resolved Hide resolved
be/CMakeLists.txt

foreach(lib IN ITEMS ${LLVM_LIBRARIES})
add_library(${lib} STATIC IMPORTED GLOBAL)
set_target_properties(${lib} PROPERTIES IMPORTED_LOCATION ${THIRDPARTY_DIR}/llvm/lib/lib${lib}.a)
Copy link
Contributor

Choose a reason for hiding this comment

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

need double check if this is always llvm/lib or possibly llvm/lib64 which is platform dependent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the installation documentation of LLVM, the default installation location for LLVM libraries is llvm/lib. To install them in llvm/lib64, specifying the "64" suffix is necessary.Additionally, I tested it on an M1 Pro Mac, and the installation location was indeed llvm/lib.

image

https://llvm.org/docs/CMake.html#:~:text=Extra%20suffix%20to%20append%20to%20the%20directory%20where%20libraries%20are%20to%20be%20installed.%20On%20a%2064%2Dbit%20architecture%2C%20one%20could%20use%20%2DDLLVM_LIBDIR_SUFFIX%3D64%20to%20install%20libraries%20to%20/usr/lib64.

thirdparty/build-thirdparty.sh Outdated
-DLLVM_INCLUDE_TESTS:BOOL=False \
-DLLVM_INCLUDE_BENCHMARKS:BOOL=False \
-DBUILD_SHARED_LIBS:BOOL=False \
-DCMAKE_BUILD_TYPE="RELEASE" \
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if cmake is case sensitive, but Release is preferred, refer to https://llvm.org/docs/CMake.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CMake is case sensitive, but for LLVM, Release and "RELEASE" have the same effect. I will modify it to Release.

@Moonm3n Moonm3n force-pushed the llvm_third_party branch 2 times, most recently from 9cd0910 to a960578 Compare August 24, 2023 14:51
@fzhedu fzhedu enabled auto-merge August 25, 2023 02:02
Moonm3n added 12 commits August 25, 2023 11:09
@Moonm3n
[Tool] Incorporate the LLVM third-party library
1d03dd7 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool] Remove the temporarily unused library
52379fe 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool] Fix llvm third-party CI error
899ac76 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool] Fix vars.sh
bd44cf0 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool] Compile the specified target only
74bed32 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool] Fix build error
2b23dd4 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
Empty commit for CI
11ed2b7 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool] Add more llvm target.
4bbeb97 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool] Optimization shell script
539ea5c 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool]: Fix CMake
d3eb372 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool] Remove debug info
0da2c5d 
Signed-off-by: Moonm3n <[email protected]>
@Moonm3n
[Tool] Restore compile flags after building llvm
2f8535e 
Signed-off-by: Moonm3n <[email protected]>
auto-merge was automatically disabled August 25, 2023 03:09

Head branch was pushed to by a user without write access

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@fzhedu fzhedu enabled auto-merge August 25, 2023 06:23
@fzhedu fzhedu merged commit 6840a50 into StarRocks:main Aug 25, 2023
@wanpengfei-git
Copy link
Collaborator

[FE Incremental Coverage Report]

😍 pass : 0 / 0 (0%)

@wanpengfei-git
Copy link
Collaborator

[BE Incremental Coverage Report]

😍 pass : 0 / 0 (0%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevincai kevincai kevincai approved these changes

@fzhedu fzhedu fzhedu approved these changes

@stdpain stdpain stdpain approved these changes

@LiShuMing LiShuMing Awaiting requested review from LiShuMing

Assignees

@Moonm3n Moonm3n

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Moonm3n @kevincai @wanpengfei-git @LiShuMing @fzhedu @stdpain