pscoro 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 ---------------- efriedma wrote: > klausler wrote: > > 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. > I think reorganizing the source-code makes sense... but I suspect it'll turn > into its own giant discussion, and it'll make it harder to review the > patches. So maybe avoid doing that for now if you can. It took a couple weeks but the support for testing in flang-rt now exists. I understand that the size of the patch has grown significantly as a result. I will work with the community closely to try to make this digestible and to address and comments or concerns. ================ Comment at: flang/runtime/sum.cpp:141 #if LDBL_MANT_DIG == 113 || HAS_FLOAT128 +#if HAS_FLOAT128 +using AccumType = __float128; ---------------- @klausler I don't plan on including this in this patch. This is related to https://reviews.llvm.org/D127025. I get an error about line 51: ``` auto next{x + correction_}; ``` when building flang-rt via the runtimes target. It complains because x is a __float128 and correction is a long double. I am unsure as to why as I am unfamiliar with what this code does. Looking at the patch linked above I saw code similar to my addition here in other files so I tried to add it here and it seemed to fix my issues. It is just a wild guess though. Please let me know if you have anymore information or intuitions regarding this bug. 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
