This is an archive of the discontinued LLVM Phabricator instance.

[flang] Fix the documentation on how to build flang
ClosedPublic

Authored by PeteSteinfeld on Jan 3 2022, 4:15 PM.

Details

Summary

I recently had an email exchange on flang-dev that revealed that the
documentation on how to build flang is incorrect. This update fixes
that.

Diff Detail

Event Timeline

PeteSteinfeld created this revision.Jan 3 2022, 4:15 PM
Herald added a project: Restricted Project. · View Herald Transcript
PeteSteinfeld requested review of this revision.Jan 3 2022, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2022, 4:15 PM
sscalpone added inline comments.Jan 3 2022, 5:08 PM
flang/README.md
75

Do you need flang here?

PeteSteinfeld added inline comments.Jan 3 2022, 6:12 PM
flang/README.md
75

Good catch. I'll add it.

Responding to Steve's comments.

PeteSteinfeld marked an inline comment as not done.Jan 3 2022, 6:14 PM

Good morning @PeteSteinfeld , thank you for making this documentation so much clearer! I have a couple of small suggestions (these are not blockers for me).

out-of-tree vs standalone
Have you considered switching to "standalone builds" instead of "out-of-tree builds"? That would be more:

  • consistent with the actual implementation/naming in the CMake script,
  • accurate, i.e. Flang is always in-tree these days (i.e. inside llvm-project).

I guess that "out-of-tree" was accurate/descriptive enough before "f18" was merged into "llvm-project". But these days Flang is always in tree. Also, "standalone" would be more consistent with other sub-projects.

CMake flags
On a different note, would it make sense to trim this set of CMake flags?

cmake \
  -G Ninja \
  ../llvm \
  -DCMAKE_BUILD_TYPE=Release \
  -DLLVM_LIT_ARGS=-v \
  -DFLANG_ENABLE_WERROR=On \
  -DCMAKE_CXX_STANDARD=17 \
  -DLLVM_ENABLE_PROJECTS="clang;mlir;flang;compiler-rt" \
  -DLLVM_BUILD_TOOLS=On \
  -DLLVM_INSTALL_UTILS=On \
  -DLLVM_TARGETS_TO_BUILD=host \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DCMAKE_CXX_LINK_FLAGS="-Wl,-rpath,$LD_LIBRARY_PATH" \
  -DCMAKE_INSTALL_PREFIX=$INSTALLDIR

Most developers want to run ninja check-flang and for that the following should be sufficient:

make \
  -G Ninja \
  ../llvm \
  -DCMAKE_BUILD_TYPE=Release \
  -DFLANG_ENABLE_WERROR=On \
  -DLLVM_ENABLE_PROJECTS="clang;mlir;flang" \
  -DLLVM_TARGETS_TO_BUILD=host \
  -DLLVM_ENABLE_ASSERTIONS=ON

Other flags are only really needed for "out-of-tree"/"standalone" builds (in order to install or build dependencies) or to make the LIT output more verbose.

I will also add @Meinersbur to the list of reviewers as he's very familiar with LLVM's build system.

flang/README.md
54

Is compiler-rt really needed? I never include it myself.

59

You could shorten the text by adding this in the bash snippet:

git clone https://github.com/llvm/llvm-project.git
cd llvm-project
59

This will add syntax highlighting. Similar comment for other code snippets in this document.

74

You can skip this, it is set in the CMake build: https://github.com/llvm/llvm-project/blob/main/flang/CMakeLists.txt#L6

110

No compiler-rt here would suggest that it's not required at all.

@awarzynski, thanks for the great feedback.

I like your suggestion of using "standalone". I'll change it.

With respect to invoking CMake and its command line, I started by spending time finding the minimal set of options that would result in a correct, testable build.

I agree that we should minimize the number CMake flags. I'll do it. But I personally always use the verbose output of "lit". It's much more useful, so I think it should be included.

flang/README.md
54

We recently found that it's needed to run executable tests in fir-dev. Including it now will avoid needing to add it later in the instructions.

59

Good suggestion about giving specific instructions for git clone. But I think it's best to separate the cloning from the rest of the build commands.

Thanks for the tip about the bash highlighting. You're a genius!

74

Thanks. Will do.

110

As I mentioned earlier, it's needed for testing flang compiled executables.

Responding to review comments.

Thank you for addressing my comments, Pete! I've left a couple more suggestions - what do you think? We could merge this as is and continue the discussion elsewhere - this is already a huge improvement! And I promise not to come back with even more ideas :)

flang/README.md
54

So:

  • llvm, mlir and clang are "build-time" dependencies (i.e. Flang won't build without them)
  • compiler-rt is a "run-time" dependency (i.e. Flang will build just fine without it, ninja check-flang will also work fine, but e.g. flang-new will fail for some more complicated examples due to missing symbols)

I think that including compiler-rt is fine, but it would be helpful to explain this distinction. In particular, compiler-rt would not be required for Flang developers who would only run ninja check-flang (as far as I know, there are no executable tests in "fir-dev"). Also buildbot maintainers could safely skip it.

By the way, have you considered using LLVM_ENABLE_RUNTIMES CMake flag here? There's been some discussion recently on LLVM_ENABLE_RUNTIMES vs LLVM_ENABLE_PROJECTS for runtime libraries:

Those discussions focused on Clang (i.e. building a Clang-based toolchain), libc++, libc++abi, libunwind (i.e. C++ runtime libraries) and compiler-rt (relevant here). We could be suggesting -DLLVM_ENABLE_RUNTIMES="compiler-rt" instead of -DLLVM_ENABLE_PROJECTS="compiler-rt" in this README. This would be more consistent with the other sub-projects and would also emphasize that compiler-rt is a slightly different dependency ("build-time" vs "run-time").

Using -DLLVM_ENABLE_RUNTIMES="compiler-rt" would mean that compiler-rt is bootstrapped using Clang that was build earlier in the build process. From the official documentation:

This is the correct way to build libc++ when putting together a toolchain

If I understand correctly, similar logic applies to compiler-rt. Having said all that, both approaches should work fine for now.

123–125

It's a bit confusing that for LLVM it's $base/build/ and otherwise it's $base/install/. If I recall correctly, you need:

  • build directory for LLVM, because that's the only location where GTest is available (required for unit tests)
  • install directory for Clang, because that's the only location were AddClang.cmake script is available (required for Clang configuration)

Fixing this would be nice, but that's totally out of scope here! Perhaps you could add a note outside this code snippet to highlight this inconsistency,

"Note that for Clang and MLIR you will use the installation directory ($base/install) and for LLVM you will need to build directory ($base/build). You can use the installation directory for all three sub-projects if you don't require the unit tests (which are configured by LLVM and are only available in the build directory)."

Otherwise this inconsistency is confusing and people might think that it's a typo. Or just miss it altogether!

PeteSteinfeld added inline comments.Jan 5 2022, 12:16 PM
flang/README.md
123–125

You're analysis is correct.

Thanks again, Andrzej for your excellent review comments.

I will add explanations for why compiler-rt is required and to note the peculiar different between the specification of the llvm and the mlir;clang;compiler-rt dependency specifications.

I'm unfamiliar with LLVM_ENABLE_RUNTIMES. I'm reluctant to document something that I don't understand.

Responding to more review comments.

Added a brief explanation for the inclusion of "compiler-rt".

@awarzynski, are there any other changes you need to approve this?

awarzynski accepted this revision.Jan 10 2022, 7:06 AM

LGTM, thank you for this!

flang/README.md
54

[nit] Would you mind adding some line feeds here before merging? People usually tweak their editors for narrower lines.

123–125

Thanks for the update! I believe that with https://reviews.llvm.org/D116731 we should be able to use $base/build for all sub-projects. We can re-visit this document once that's merged.

130

[nit] Would you mind adding some line feeds here before merging? People usually tweak their editors for narrower lines.

This revision is now accepted and ready to land.Jan 10 2022, 7:06 AM
rovka added inline comments.Jan 10 2022, 7:37 AM
flang/README.md
101

I have 2 mostly orthogonal comments here:

  1. People might skip directly to this section and then be surprised a few lines down when you mention the in tree build. I think this paragraph should make it clear from the start that you're expected to go through the previous section first. We should probably also mention that if you're building standalone you no longer need to include flang in LLVM_ENABLE_PROJECTS for the in tree build (otherwise people will end up building flang twice when they're really only interested in the standalone one).
  1. Do we really need to clone again? It should work to just point cmake to the flang subdirectory inside the first clone.
PeteSteinfeld added inline comments.Jan 10 2022, 9:03 AM
flang/README.md
101

Thanks for the feedback, Diana.

Good point about needing to tell people that building in tree is a prerequisite for building standalone. I'll add some text.

I originally did not include flang in the list of projects for the in tree build, but an earlier reviewer requested it. In fact, my normal practice is to include it, if only to be able to run the flang tests and verify that the source code is good. I'll add a note to capture this.

You don't need to clone again. But that's my normal practice. This is because I typically create one base, in tree build and then create several standalone builds that all use that base builds. Typically, these stand-along builds have different Git repositories than the base build.

Responding to Diana's comments.

As per Andrzej's request, I've updated the text to fit into 80 columns.

This revision was automatically updated to reflect the committed changes.