Page MenuHomePhabricator

Add new LLVMComponents CMake module.
Needs ReviewPublic

Authored by stellaraccident on Jan 11 2021, 12:02 AM.

Details

Summary
  • Ultimately will obviate the need for llvm_add_library and its derivitives.
  • This is non functional at head, but I have a private branch where I have all LLVM/MLIR tests passing with pragmatic patches to AddLLVM.cmake and LLVM-Config.cmake (and some lists).
  • Goal is to land this and continue iterating on more principled cleanup to the other parts until things are close enough to switch to this facility.
  • There are still some llvm_add_library features missing here. They will be added as things progress.
  • As discussed in RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-January/147567.html

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Jan 11 2021, 12:02 AM
llvm/cmake/modules/LLVMComponents.cmake
17

Note: it took several iterations on the codebase to get it to the point where the component interface target has the component name (there were a number of conflicting usages), but now that it does, I can remove this alias, in favor of just directly linking against the component name (ie. LLVMSupport). I'll remove the alias in a follow-up (it is rather delicate to keep the codebase building as I change things here).

First review round. I'm pretty excited by this work, thanks for handling this!

As a side note, I'd feel much more comfortable landing this patch if there was an actual use of it in the codebase, even for just one component. Other wise it can land and stay there, we have no way to ensure it works as expected. Maybe we already have a component that matches your model?

llvm/cmake/modules/LLVMComponents.cmake
125

I've see ${libname}_obj(s) used a lot across cmakefiles to carry that meaning.

148

Do we want to normalize upper/lower case here?

166

Out of curiosity, why NEWCOMPONENT and not COMPONENT

189

I which situation can it already exist? And if it does should we check it's not already *populated* ?

218

The two calls above are identical, and also similar to the calls on ${_impl_target}. Maybe syndicate them in a function, or in a loop?

254

[nit] I'd rather use an if() / else() here.

400

instead of providing this function, we could provide a get_target_component_property(component_name...) that would make the _props` an implementation detail?

440

Portability issue: LLVM is supposed to compile with MSVC, and it doesn't support that flag.

Include minimal changes to the tree to build/test.

Hi serge-sans-paille, thank you for the comments! They caught me mid-cleanup so, I decided to push a revision with the further minimal changes needed to get the build mostly functional with this facility, as I think it might provide some better context. Now that I've got the tree in a non-hacky building mode and just have a handful of failing tests (around llvm-config mostly), I'll work through your comments a bit.

Note that as I was making the updates, the stricter arrangement actually flagged most of the issues, so finding/fixing was not too bad (for this most recent iteration -- my previous attempts were very hacky and hard). As I was making changes, I took notes on the things that still need doing: https://gist.github.com/stellaraccident/47b3ed28b8134bb0b6db2afbec7fc789

I'm still not exactly sure what the right development flow for this kind of change is. My current thought is to spend a few more iterations to make it fully functional (right now LLVM and MLIR build in both static and dynamic mode on Linux with just an X86 target, and just a few llvm-config tests are failing (expected)), and keep tracking head as I do it. Then when it seems in reasonable shape, see what dimensions we can slice it on safely for submission. Prior to that last part, I will need more community involvement to identify platforms and configurations that are important to validate.

Also added file sizes for binaries and libraries in unbundled mode to the above gist.

tstellar added inline comments.Jan 19 2021, 7:28 AM
llvm/lib/ExecutionEngine/JITLink/CMakeLists.txt
27–33

Can this be its own patch or does it depend on other changes in this patch?

llvm/lib/ExecutionEngine/Orc/CMakeLists.txt
51–58

Same question here, and for some of these other changes that look like cleanups.

llvm/lib/ExecutionEngine/JITLink/CMakeLists.txt
27–33

I've not yet gone through and picked these individual changes into head. I agree: a number of them likely do not depend on the other changes but were merely forced by it. Just takes some time to identify that for real.

stellaraccident marked 3 inline comments as done.

Address comments and rebase.

PTAL - I ran out of time this week to make substantive progress on this, but I took a pass through the comments and cleaned up. When I next have time, I'll work on finishing the dev work on it to get everything working, then integrate it into the existing CMAKE flags and checkpoint to see where we are.

llvm/cmake/modules/LLVMComponents.cmake
148

Ok - took a pass through and normalized.

166

Added:

# TODO: Rename "NEWCOMPONENT" -> "COMPONENT" once migration is complete.
# Just choosing a different name to avoid collisions with the existing
# implementation until done.

I had issues early on with finding old code that used same named properties. I'll clean this up a bit later once I'm sure it is all gone.

189

For multi-library components (currently includes all targets but there are other cases in MLIR). The first one through defines it. Added a comment. Since this is also the code that populates it, not sure how much more verification we want to do... future us may appreciate a nice error here if someone collides in the namespace with a non-component library. Wdyt?

400

Good idea. Made that change. Will probably also need to add a setter once I finish the porting. Currently this is used for SYSTEM_LIBS and I have that commented out.

440

Ah - I just ripped that off from somewhere while waiting to find where to shove a dummy.cpp file. Went looking and found one already there so updated everything to use it. It has a typedef in it so as to avoid triggering this warning.

llvm/cmake/modules/AddLLVM.cmake
700–701

If I understand correctly, you're now defering this to llvm_component_add_library ? In that case this should be reflected somewhere, otherwise you're loosing some component information here.

716

I like this change. You'd need to update the documentation to reflect that this is now a normal component

1473

I agree with this diff, but given the size of this patch, it's probably better *not* to fix cosmetic stuff ;-)

llvm/lib/CMakeLists.txt
59–60

It's super nice that you manage to remove this post-processing step!
But removing the LLVMBuildGenerateCFragment puzzles me... I don't see it being generated elsewhere :-)

llvm/lib/ExecutionEngine/Orc/CMakeLists.txt
35

I <3 this kind of change.

llvm/lib/Support/CMakeLists.txt
264

I don't see where _prop_target is set o_O

llvm/tools/llvm-config/CMakeLists.txt
12–14

This is much cleaner, thanks!

llvm/unittests/Target/X86/CMakeLists.txt
14

Cool!

stellaraccident marked 3 inline comments as done.

Rebase and fix llvm-config.

Addressed a couple of comments and knocked llvm-config off of my todo list. It will need a couple more tweaks once I fix some of the other shared lib flags but is mostly as it should be (and tests pass now).

llvm/cmake/modules/AddLLVM.cmake
1473

Ah, didn't realize I still had that diff. I believe I was doing an edit there that I backed out (after reformatting it).

llvm/lib/CMakeLists.txt
59–60

There are a number of things on my todo list, and this was one. Pushing an update momentarily that fixes llvm-config. Here is my todo list: https://gist.github.com/stellaraccident/47b3ed28b8134bb0b6db2afbec7fc789#file-todo-md

Remove some AddLLVM things no longer used.

Make fully static and BUILD_SHARED_LIBS mode work.

Still working on libLLVM.so building.

FYI - Update has been a while. I am still working on this: I hit a snag with libLLVM which has required some rework. I think I can see the light at the end of the tunnel and hope to send an updated version soon.