klausler 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: > > > > 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.) > If we are decoupling the affected tests from flang-rt, then I think the only > component left coupled (that I'd argue shouldn't be coupled) is the runtime > source files. I'm seeing that for all the other runtimes in the llvm-project, > the sources exist in a top level directory. Would it make sense and be > feasible for flang-rt to act similarly? > > Comparing to the infrastructure for compiler-rt, I've been entertaining the > idea that FortranRuntime and FortranMain can be treated as a library with > sources defined in `flang-rt/lib/FortranRuntime` and > `flang-rt/lib/FortranMain`, similar to how compiler-rt has libraries like > `compiler-rt/lib/asan` that are added to the final compiler-rt target. > > FortranDecimalRT's sources would still be gathered from the Flang source > directory, which would have to be known by flang-rt (also I see that multiple > runtime files include `flang/Common` headers (but no sources get linked), so > flang-rt would need to be aware of flang's directory for that reason too) > > Do you think this idea worth pursuing or has the scope expanded way past the > original intention? This would make life more difficult for people (okay, me) doing most of their flang development in an out-of-tree mode. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154869/new/ https://reviews.llvm.org/D154869 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
