This is an archive of the discontinued LLVM Phabricator instance.

Update all LLVM documentation mentioning runtimes in LLVM_ENABLE_PROJECTS
ClosedPublic

Authored by ldionne on Feb 9 2022, 9:34 AM.

Details

Summary

We are moving away from building the runtimes with LLVM_ENABLE_PROJECTS,
however the documentation was largely outdated. This commit updates all
the documentation I could find to use LLVM_ENABLE_RUNTIMES instead of
LLVM_ENABLE_PROJECTS for building runtimes.

Note that in the near future, libcxx, libcxxabi and libunwind will stop
supporting being built with LLVM_ENABLE_PROJECTS altogether. I don't know
what the plans are for other runtimes like libc, openmp and compiler-rt,
so I didn't make any changes to the documentation that would imply
something for those projects.

Once this lands, I will also cherry-pick this on the release/14.x branch
to make sure that LLVM's documentation is up-to-date and reflects what
we intend to support in the future.

Diff Detail

Event Timeline

ldionne created this revision.Feb 9 2022, 9:34 AM
Herald added a reviewer: rafauler. · View Herald Transcript
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested review of this revision.Feb 9 2022, 9:34 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits and 3 others. · View Herald Transcript

BOLT modifications LGTM, thanks

Quuxplusone added inline comments.
bolt/docs/OptimizingClang.md
229–236

FWIW, I personally prefer to run cmake and ninja from the build directory, i.e.

$ mkdir ${TOPLEV}/llvm-project/llvm/stage2-prof-gen
$ cd ${TOPLEV}/llvm-project/llvm/stage2-prof-gen
$ CPATH=${TOPLEV}/stage1/install/bin
$ cmake -G Ninja \
    -DLLVM_TARGETS_TO_BUILD=X86 \
    ~~~
    -DCMAKE_INSTALL_PREFIX=${TOPLEV}/stage2-prof-gen/install \
    ../llvm-project/llvm
$ ninja install

This is the style that was used in DataFlowSanitizer.rst below, which you explicitly moved away from into -B/-S/-C land (where I find it harder to keep straight all the different extra options that are needed).

Orthogonally, I'm worried that the advice on lines 73–74 of README.md doesn't mention CMAKE_INSTALL_PREFIX. When I'm building libc++ locally as part of my development workflow, I absolutely do not want to blow away my computer's default standard library installation; but what do I want to do? Should I use something like -DCMAKE_INSTALL_PREFIX=$(pwd)/install as depicted here? Can I really not get away with "running it out of the buildroot" the way I'm used to?— I must install it somewhere on my system in order to test it at all?

flang/README.md
36–37
MaskRay accepted this revision.Feb 9 2022, 4:33 PM
MaskRay added a subscriber: MaskRay.

Thanks for improving the documentation. I occasionally see folks (and two today!) puzzled by LLVM_ENABLE_RUNTIMES and this patch should help them.

Personally I use the -S -B style (actually -H, but now I see that -S is documented I will switch).
I sometimes twiddle with a cmake file and spawn multiple builds.
Not changing the work directory makes my commands simpler, but I do see @Quuxplusone's point about -DCMAKE_INSTALL_PREFIX=$(pwd)/install.

I rarely use CMAKE_INSTALL_PREFIX. When I need it, I accept the slight inconvenience of duplicating the build directory twice for -B and -DCMAKE_INSTALL_PREFIX.

I have read the updated and they look good to me, but happy if we switch to @Quuxplusone's style.
I passes the link to some folks and hope that fresh eyes with less LLVM_ENABLE_RUNTIMES experience can make suggestions.

tianshilei1992 added inline comments.
README.md
71

FWIW, OpenMP can be put into either of them. In OpenMP doc (https://openmp.llvm.org/SupportAndFAQ.html#q-how-to-build-an-openmp-gpu-offload-capable-compiler), we recommend to use LLVM_ENABLE_RUNTIMES to build OpenMP if users would like to use offloading features.

ldionne updated this revision to Diff 407500.Feb 10 2022, 5:35 AM
ldionne marked 3 inline comments as done.

Address review comments.

README.md
71

Thanks for the clarification, I tweaked the documentation a bit.

bolt/docs/OptimizingClang.md
229–236

Re: -C -B -S

I really like using those flags because it makes the command relocatable, i.e. it works regardless of where it is being run. As such, it's way more copy-pasteable too. However, since that's orthogonal to this change, I'll revert to the old style just so we have fewer things to argue about. But in general, for libc++/libc++abi/libunwind instructions, you'll find that I want things documented this way for that reason.

Re: CMAKE_INSTALL_PREFIX

You don't necessarily need to install it in order to test it -- we test libc++ all the time without running the install-cxx target. Furthermore, I would expect that most systems nowadays have some sort of integrity protection that prevents you from doing something bad like that. Apple platforms certainly do -- you simply can't overwrite your libc++.1.dylib anymore and render your system unusable. Still, I'll add a word of caution to CMAKE_INSTALL_PREFIX above even though it's orthogonal to this patch -- it's easy enough to do.

ldionne accepted this revision as: Restricted Project.Feb 10 2022, 5:36 AM
This revision is now accepted and ready to land.Feb 10 2022, 5:36 AM
bolt/docs/OptimizingClang.md
229–236

FWIW, the CMAKE_INSTALL_PREFIX change is an improvement but only slightly more reassuring than before. ;) Part of my paranoia is that I happen to know that Homebrew stores things in /usr/local/... as well (although probably entirely under /usr/local/Cellar/...?) and I wouldn't want to blow those away either.
Apparently on my Apple system I have /usr/lib/libc++.dylib, /System/DriverKit/usr/lib/libc++.dylib, and /usr/local/Cellar/llvm/12.0.1/lib/libc++.dylib.

we test libc++ all the time without running the install-cxx target

Well, I know I do, but I don't use ENABLE_RUNTIMES yet... and the how-to below does end with the line ninja -C build install-cxx. If there's a supported/expected-to-work way to build and test libc++ that doesn't use ninja install-cxx, it'd be cool to mention it.

Anyway, this is all just me fudding and shouldn't block this PR. :)

llvm/docs/GettingStarted.rst
633–634

The other list is a-z order; this one could be too.

ldionne updated this revision to Diff 407631.Feb 10 2022, 12:02 PM
ldionne marked 2 inline comments as done.

Address review comments. I'm going to commit this and cherry-pick onto release/14.x

bolt/docs/OptimizingClang.md
229–236

I think your comments are valid, but I think I would defer to @rafauler if they want to change the Bolt documentation in that way.

Well, I know I do, but I don't use ENABLE_RUNTIMES yet...

FWIW, LLVM_ENABLE_RUNTIMES won't change anything with respect to that.

This revision was landed with ongoing or failed builds.Feb 10 2022, 12:05 PM
This revision was automatically updated to reflect the committed changes.
Arfrever added inline comments.
llvm/docs/GettingStarted.rst
1225

One space is missing before *, so currently this part of this page is rendered incorrectly. See https://github.com/llvm/llvm-project/blob/main/llvm/docs/GettingStarted.rst (close to the end).

(When you fix it, please also backport fix to branch release/14.x.)

ldionne marked an inline comment as done.Feb 11 2022, 6:16 AM
ldionne added inline comments.
llvm/docs/GettingStarted.rst
1225

Thanks, good catch.