This is an archive of the discontinued LLVM Phabricator instance.

[docs] Improve documentation around CMAKE_BUILD_TYPE
ClosedPublic

Authored by thieta on Apr 25 2022, 1:31 AM.

Diff Detail

Event Timeline

thieta created this revision.Apr 25 2022, 1:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2022, 1:31 AM
Herald added a subscriber: mgorny. · View Herald Transcript
thieta requested review of this revision.Apr 25 2022, 1:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2022, 1:31 AM

Thanks for driving this.

tschuett added inline comments.Apr 25 2022, 1:37 AM
llvm/docs/CMake.rst
208
209
jhenderson added inline comments.
llvm/docs/CMake.rst
208

I suspect RAM is the limiting factor for most users rather than disk space. I'd suggest mentioning that here too.

209–210

Let's be a bit more explicit by stating the sort of issues (i.e. OOM issues) when doing this build.

222
223

Here and possibly in a few other places, you've referred to builds in the plural, whereas usually you refer to it in the singular. I don't mind which, but you should be consistent at least.

llvm/docs/GettingStarted.rst
77–78

Do you mean "with it's libraries" i.e. someone who is developing a project using LLVM libraries? If not, it should be "or it's libraries".

1191

The build types control three different aspects, so maybe you could even present this as a table:

Build typeOptimizationsDebug InfoAssertions
ReleaseFor speed
DebugNone
RelWithDebInfoFor speed/size
MinSizeRelFor size

Then briefly explain the aspects:

  • Optimizations make code run faster but can be an impediment to step-by-step debugging.
  • Builds with debug info can consume lots of memory and disk space, here is how that might be mitigated: ...
  • Assertions are internal constraint checks that can help find bugs.

Then you don't have to repeat wording about those aspects.

llvm/docs/CMake.rst
202–203

Perhaps we can phrase this in a way that doesn't suggest we know what others want, like "It is best suited for users of LLVM and Clang."

206–207

Let's keep it short and say "disables optimizations". That's pretty much true for -O0.

207
207–208

I don't think they are necessarily slower. Optimizations can also take some time, and perhaps under certain circumstances this is no longer true.

208

The build might not run out of disk space, but it can be very IO-heavy (without split debug info).

But memory is the main issue of course.

208

Perhaps we can add that these are most suitable for step-by-step debugging.

208–211

I wouldn't suggest a specific linker here, but simply list some options to mitigate the downsides: LLVM_USE_LINKER (LLVM_ENABLE_LLD is mostly interesting for two-stage builds, and gold is also an option), LLVM_USE_SPLIT_DWARF, LLVM_PARALLEL_LINK_JOBS, and BUILD_SHARED_LIBS. There is no need to explain them, they are explained below.

217

It's more of a combination than a balance, as the name implies: the optimizations are practically the same as in Release (the difference between -O2 and -O3 is marginal in Clang), and we have full debug info as in Debug.

218–221

Here I would suggest something like "Binaries will be fast as in Release, but it allows limited interactive debugging".

219–221

True, but since we want people new to the code to read that, I'd suggest to omit that in the sense of brevity.

227–228

Also here: I'd suggest to omit that in the interest of keeping it short. This build type is rarely used anyway.

llvm/docs/GettingStarted.rst
75–80

Doesn't this duplicate the section below? There is certain danger in having different parts of the documentation talking about the same stuff. I'd prefer if you just pointed there as in "For details see below", or kept the reference to CMAKE_BUILD_TYPE.

101

Why is that # needed now?

1190–1191

The most important aspect is already there, so maybe just a brief addition. Also we don't need to recommend an alternative, since it's right after this. Detailed information can be found in the CMake page.

Thanks for the suggestions all - I am working on this and came up with this table, WDYT. I will upload this as a diff when I have addressed all the rest of the comments, but wanted to get the input on the table first since it's a bit annoying to mark-up correctly.

Thanks! I have the feeling that Release wins at execution speed and is the fastest to build?

This looks pretty good!

Table looks good to me, although the second bullet has some grammar errors in it. Is it worth referencing the cmake option to enable assertions in the bullet about assertions?

thieta updated this revision to Diff 425730.Apr 28 2022, 2:48 AM

Review updates, make it a more readable table and point more people to
this table

Here is my latest attempt:

Did a table, then pointed all references to CMAKE_BUILD_TYPE to this table so that we keep all documentation about it in the same place.

Please take another pass on it - and feel free to point out my grammar errors (English is not my first language).

jhenderson added inline comments.Apr 28 2022, 2:52 AM
llvm/docs/CMake.rst
212
llvm/docs/GettingStarted.rst
77–78

Not addressed? Also it's -> its.

thieta added inline comments.Apr 28 2022, 3:10 AM
llvm/docs/GettingStarted.rst
77–78

I did change it to or it's libraries, will fix the other issue.

thieta updated this revision to Diff 425738.Apr 28 2022, 3:21 AM

Smaller grammar fixes

jhenderson added inline comments.Apr 28 2022, 3:23 AM
llvm/docs/GettingStarted.rst
77–78

You didn't address the main bit which was the inline edit (use "which" and "who" instead of "that").

thieta added inline comments.Apr 28 2022, 3:25 AM
llvm/docs/GettingStarted.rst
77–78

Ah shit - thanks for pointing that out. I was focused on the it's bit. I find the inline edits hard to see when you are color blind at times. Will fix in the next update.

thieta updated this revision to Diff 425978.Apr 28 2022, 11:30 PM

Finally fix this line

jhenderson added inline comments.Apr 29 2022, 1:45 AM
llvm/docs/CMake.rst
209
211–212

"can use a lot of RAM and disk space and are usually"

215–216

Nit: possibly wrapped a bit prematurely?

llvm/docs/GettingStarted.rst
1191–1194

This paragraph seems to be a match for the earlier one under "common cmake options". If you feel it should be duplicated, please update it to fix the same grammar issues.

(i.e. "that" -> "which")

thieta updated this revision to Diff 426340.May 1 2022, 11:46 PM
thieta marked 3 inline comments as done.

Latest batch up of updates

Thanks for all your detailed feedback @jhenderson

thieta marked 26 inline comments as done.May 1 2022, 11:48 PM
jhenderson accepted this revision.May 3 2022, 12:11 AM

LGTM, but worth waiting for others to take a look.

This revision is now accepted and ready to land.May 3 2022, 12:11 AM

Thanks everyone for the input! If you have any more input on this - please try to get to it today since I plan to merge this and the CMake change to show this documentation tomorrow CEST.

awarzynski added inline comments.May 3 2022, 1:13 AM
llvm/docs/GettingStarted.rst
51

I think that the moment you land https://reviews.llvm.org/D124153, this line will be invalid.

thieta added a comment.May 4 2022, 2:23 AM

I will land this documentation update now since it's pretty low risk, and I want to land before the other diff making CMAKE_BUILD_TYPE mandatory. I'll be happy to make more changes if people feel it's needed. Hit me up with other changes in that case.

This revision was automatically updated to reflect the committed changes.