Page MenuHomePhabricator

Add new LLVMComponents CMake module.
Needs ReviewPublic

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


  • 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:

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Jan 11 2021, 12:02 AM

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?


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


Do we want to normalize upper/lower case here?


Out of curiosity, why NEWCOMPONENT and not COMPONENT


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


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?


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


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


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:

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

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


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


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.


Ok - took a pass through and normalized.



# 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.


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?


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.


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.


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.


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


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


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 :-)


I <3 this kind of change.


I don't see where _prop_target is set o_O


This is much cleaner, thanks!



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).


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


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:

Remove some AddLLVM things no longer used.

Make fully static and BUILD_SHARED_LIBS mode work.

Still working on 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.