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

Tue Aug 15 10:47:54 PDT 2023

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