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

[CMake/Bazel] Support usage of opt driver as a library #79205

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

wsmoses
Copy link
Member

@wsmoses wsmoses commented Jan 23, 2024

In Bazel, Clang current separates the clang executable into a clang-driver library, and the actual clang executable. This allows downstream users to make their own variations of clang, without having to redo/maintain separate build pipelines.

This adds the same for opt for both CMake and Bazel.

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

If we're going to make opt-driver a library, it should be a library for both CMake and Bazel builds. The set of available libraries shouldn't depend on how you build LLVM.

@wsmoses
Copy link
Member Author

wsmoses commented Jan 24, 2024

I'm fine with that but FYI I emulated the clang approach, which does have this in Bazel (

) but not CMake ( )

@wsmoses
Copy link
Member Author

wsmoses commented Jan 24, 2024

@efriedma-quic added a CMake Library for opt. I presume we should add one for clang as well, but that should probably be a separate PR.

@wsmoses wsmoses force-pushed the optdriver-bazel branch 2 times, most recently from 12ede4b to bf11098 Compare January 24, 2024 00:31
@efriedma-quic
Copy link
Collaborator

Looks okay to me, but I'm a bit worried there's something I'm missing, so I'd like a second reviewer to chime in.

@wsmoses wsmoses force-pushed the optdriver-bazel branch 6 times, most recently from ec2e2e2 to 2ead782 Compare January 24, 2024 01:23
@MaskRay
Copy link
Member

MaskRay commented Jan 24, 2024

This allows downstream users to make their own variations of clang, without having to redo/maintain separate build pipelines.

Consider naming the project, so if other people do any change, they can find the project name from the commit message...

It may be worth testing the following configurations:

default
-DBUILD_SHARED_LIBS=on
-DLLVM_LINK_LLVM_DYLIB=on

@wsmoses wsmoses force-pushed the optdriver-bazel branch 4 times, most recently from eb21d98 to d2f8cdd Compare January 24, 2024 02:47
@wsmoses
Copy link
Member Author

wsmoses commented Jan 24, 2024

@efriedma-quic @MaskRay when I did the build matrix of options unfortunately the DBUILD_SHARED_LIBS one kept running into issues, so I refactored the opt library in the way shown for all of CI to succeed.

@efriedma-quic note, since I had to make a separate driver function, I made it future compatible for #79227. Once this lands, I will rebase the new test/driver in that PR on this.

@GMNGeoffrey GMNGeoffrey removed their request for review January 24, 2024 03:25
@wsmoses wsmoses changed the title [Bazel] Support opt-driver library, like clang-driver [CMake/Bazel] Support usage of opt driver as a library Jan 24, 2024
@wsmoses wsmoses merged commit 32f7922 into llvm:main Jan 24, 2024
3 of 4 checks passed
@wsmoses wsmoses deleted the optdriver-bazel branch January 24, 2024 18:39
add_llvm_tool(opt
# We don't want to link this into libLLVM
add_llvm_library(LLVMOptDriver
STATIC
Copy link
Contributor

Choose a reason for hiding this comment

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

The usual llvlm style is to have one target per directory. If this is now a lib, could you move this into llvm/tools/opt/lib? Then you don't need the PARTIAL_SOURCES_INTENDED below.

@nico
Copy link
Contributor

nico commented Jan 24, 2024

Also, this doesn't build, see e.g. here: https://lab.llvm.org/buildbot/#/builders/217/builds/35051

nico added a commit that referenced this pull request Jan 24, 2024
@nico
Copy link
Contributor

nico commented Jan 24, 2024

Reverted in be08be5 for now.

@wsmoses
Copy link
Member Author

wsmoses commented Jan 24, 2024 via email

@nico
Copy link
Contributor

nico commented Jan 24, 2024

Whoops, sorry, the build failure was due to something else! I'll reland, apologies. Please do move the lib to its own dir though.

nico added a commit that referenced this pull request Jan 24, 2024
This reverts commit be08be5.
The build error was due to a different change, apologies!
@nico
Copy link
Contributor

nico commented Jan 24, 2024

Relanded in 3135984. Apologies again.

@wsmoses
Copy link
Member Author

wsmoses commented Jan 24, 2024

Will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants