[PATCH] D116566: [flang] Fix the documentation on how to build flang

Wed Jan 5 08:27:43 PST 2022

awarzynski added a comment.

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

================
Comment at: flang/README.md:123-125
+  -DLLVM_DIR=$base/build/lib/cmake/llvm \
+  -DCLANG_DIR=$base/install/lib/cmake/clang \
+  -DMLIR_DIR=$base/install/lib/cmake/mlir \
----------------
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!

================
Comment at: flang/README.md:54
+Building flang in tree means building flang along with all of the projects on
+which it depends.  These include llvm, mlir, clang, and compiler-rt.
+
----------------
PeteSteinfeld wrote:
> awarzynski wrote:
> > Is `compiler-rt` really needed? I never include it myself.
> 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.
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` [[ https://llvm.org/docs/CMake.html | CMake flag ]] here? There's been some discussion recently on `LLVM_ENABLE_RUNTIMES` vs `LLVM_ENABLE_PROJECTS` for runtime libraries:
* https://lists.llvm.org/pipermail/llvm-dev/2021-October/153238.html
* https://github.com/ClangBuiltLinux/llvm-distributors-conf-2021/blob/main/slides/LLVM%20Runtimes%20Build.pdf and https://www.youtube.com/watch?v=UMDRAmmDBgM
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 [[ https://llvm.org/docs/CMake.html | 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116566/new/

https://reviews.llvm.org/D116566