[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: flang/runtime/CMakeLists.txt:251
 
-  INSTALL_WITH_TOOLCHAIN
-)
+if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES)
+  add_flang_library(FortranRuntime STATIC
----------------
pscoro wrote:
> efriedma wrote:
> > pscoro wrote:
> > > efriedma wrote:
> > > > This "if" doesn't make sense to me.  If we're not building flang-rt, we shouldn't be here, so I don't see why you need an "if" in the first place.
> > > `add_subdirectory(runtime)` is a line that still exists in `flang/CMakeLists.txt`. This exists because `Fortran_main` is still being built at the same time as the compiler, and to do so, the runtime subdirectory still needs to be added to flang (`flang/CMakeLists.txt` -> `add_subdirectory(runtime)` -> `flang/runtime/CMakeLists.txt` -> `add_subdirectory(FortranMain)`. The solution I had was to just add a check around the `FortranRuntime` library production so that it only happens for flang-rt.
> > > 
> > > If you have a better solution let me know. Overall, I'm not sure if Fortran_main is currently being handled in the best way (ie, its still being built at the same time as the compiler, which doesn't seem ideal), but am not sure what course of action to take with it since it doesn't really belong in flang-rt either (see documentation for details)
> > Fortran_main should be "part of" flang-rt in the sense that building flang-rt builds it.  Most of the same reasons we want to build flang-rt.a as a runtime apply.
> > 
> > Since the output needs to be separate, building flang-rt should produce two libraries: flang-rt.a and FortranMain.a.
> I agree with this idea and have been trying to make it work but in the process I discovered that my "fix" above (`if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES)`) is worse than I thought.
> 
> By building the llvm target with flang-rt as an enabled runtime, we are essentially saying: build the flang compiler, and then invoke cmake again to build the runtimes project (externally), which builds flang-rt.
> 
> The problem is that check-all depends on check-flang which depends on the runtime. The if guard above was not actually doing what I thought it was, and the reason why configuring didnt fail with "flang-rt does not exist" is because the if guard was still evaluating to true. Basically, the guard ensured that FortranRuntime was only being built if flang-rt is built, but the add_library call was still being reached *during the configuration of the flang compiler*.
> 
> I am having trouble dealing with this dependency since runtimes get built externally (and after the compiler is built, which is when the check-all custom target gets built). I will have to investigate more closely how other runtimes handle this. My initial thought was perhaps the runtime testing should be decoupled from check-flang/check-all however I don't know if that's a good idea, and I also think that only helps with flang-rt, but if Fortran_main is required to build any executable I imagine that it should be needed for check-flang?
> 
> This is just an update of whats holding me back right now. If you have any tips please let me know. Thanks for bringing this to my attention.
For comparison, check-clang doesn't require any runtime libraries at all. All the checks only check the generated LLVM IR/driver commands/etc.  Any tests that require runtime execution go into either the regression tests for the runtimes themselves, or into lllvm-test-suite.

Probably check-flang should do something similar. It probably makes sense to add a check-flang-rt target or something like that.  (Not sure which existing flang tests are impacted.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154869