[flang-rt] Implement iso module in C++ in the runtime - #197687
Conversation
|
libc++ test failure is unrelated, everything is green. |
|
Will the dependency come back anyway once @Meinersbur move the module files to the runtime? |
1dae0b5 to
7e9c023
Compare
You are correct, so I have removed the part of the code that removes the flang setup and dependency. However, I believe this PR is still valuable. The incoming changes in #171515 will move module generation here, but this file is different as it creates real symbols. With this PR, we can logically split The main motivating factor here is that internally at AMD we are struggling with the extremely heavy resource requirements for building flang, so the effort is to contain it as much as possible. We are going to limit the dependency graph such that we can build as much as possible without |
79a809d to
5e9f3a2
Compare
I was actually trying to go the other way. It seems obvious to be able to implement (parts of) a language's runtime in the language itself, in the sense that I don't know any other language that forces its runtime to be entirely written in another language. There have been several attemps already: #169525 (since the reason why With Of course, a complete Fortan toolchain, including running |
5e9f3a2 to
9d99489
Compare
This is my plan, basically. The changes in #198205 set up the dependencies so that we only need to build |
Is there a way to verify that the C++ implementation and the Fortran implementation of the module provide "the same" binary automatically at least in some flang/flang-rt builds? I am worried about future changes causing misalignment between the two implementations. |
There's not an easy way to do this as far as I'm aware. We'd need a separate test that compiles the |
I am okay with using the C++ implementation for the development purposes, but I think it will be better if we use the actual Fortran module for the complete toolchain build (i.e. the one that will probably be used for the official release builds). Will it be possible to only use the C++ implementation under an explicit CMake option? |
That doesn't really solve my problem. For context, we at AMD are dealing with several issues that stem from the We will still have the module file step, and #171515 will move that here as well (which will also help keep these in sync). It's just that the linkable symbols are in |
|
Isn't it possible to have dependency on flang and the Fortran module in one configuration and do not have it in another? What I am suggesting is to add a CMake option like What am I missing? |
Is the suggestion that we have two paths? I don't think it is worth the complexity. I will try to get a comprehensive test working so that we can prove the symbols always match up. I think that will ease a lot of anxiety. |
Summary: Right now we have a very heavy dependency on `flang` for a single module file. The flang build will handle creating the actual module file, we just need to export the symbols. This PR ports the source file from the flang .f90 to equivalent C++. This allows us to build `flang-rt` standalone without bringing in the entire flang infrastructure, which is by far the heaviest build in the entire LLVM project. I verified the symbols and values against a previous build, so they are identical.
I am not looking forward into maintaining two redundant versions. Generally, contributors also rarely think about updating configurations that they do not use. By experience, those go out of maintanance/grow inconsistencies over time. For iso_fortran_env in particular, if this is about consistency with Flang's understanding of these kinds, I think a unittest would be the best approach. |
5567883 to
206d3df
Compare
|
Added a test, it should fail if these every get out of sync. |
Right, I have the same concern about the two implementations of the module symbols. Thank you for adding the test! LGTM |
|
Going to go ahead and merge this. I don't think this precludes us from having Fortran sources in the future but I think we should think explicitly about how they're partitioned. This makes it easier to make the 99.9% C++ file 100% C++. |
| ! RUN: | grep '_QMiso_fortran_env_impl' | cut -d' ' -f1,4 | sort > %t.cpp.syms | ||
|
|
||
| ! The two symbol lists must be identical. | ||
| ! RUN: diff %t.f90.syms %t.cpp.syms |
There was a problem hiding this comment.
I am getting the following failure from this PR:
# RUN: at line 13
diff ...
# .---command stdout------------
# | *** compare_iso_fortran_env_symbols.f90.tmp.f90.syms
# | --- compare_iso_fortran_env_symbols.f90.tmp.cpp.syms
# | ***************
# | *** 1,6 ****
# | _QMiso_fortran_env_implEC__builtin_integer_kinds 14
# | _QMiso_fortran_env_implEC__builtin_logical_kinds 10
# | ! _QMiso_fortran_env_implEC__builtin_real_kinds 18
# | _QMiso_fortran_env_implECbfloat16 4
# | _QMiso_fortran_env_implECint128 4
# | _QMiso_fortran_env_implECint16 4
# | --- 1,6 ----
# | _QMiso_fortran_env_implEC__builtin_integer_kinds 14
# | _QMiso_fortran_env_implEC__builtin_logical_kinds 10
# | ! _QMiso_fortran_env_implEC__builtin_real_kinds 14
# | _QMiso_fortran_env_implECbfloat16 4
# | _QMiso_fortran_env_implECint128 4
# | _QMiso_fortran_env_implECint16 4
# `-----------------------------
# error: command failed with exit status: 1
There was a problem hiding this comment.
Ah, sorry, what's your platform? At least the test is doing its job.
There was a problem hiding this comment.
My guess is that it might be a platform without 128bit float and for some reason the original ISO file says it's supported.
There was a problem hiding this comment.
Our flang-rt is built with -DFLANG_RT_SUPPORTS_REAL16=1, so it contains the real-kind-16 in the array. The test's clang command does not specify this macro, so the real-kind-16 is missing from the *.cpp.o file.
There was a problem hiding this comment.
I wonder if it is possible to compare the module compilation with the real iso_fortran_env_impl.cpp.o that was built during the library build.
There was a problem hiding this comment.
llvm-nm --defined-only --format=posix --print-size ./lib/clang/23/lib/x86_64-unknown-linux-gnu/libflang_rt.runtime.* | grep iso_fortran_env_impl seems to work for shared and static libs
There was a problem hiding this comment.
That's what I was thinking, but if we build with both it would double up all the symbols. Maybe I can just run it through uniq?
There was a problem hiding this comment.
I guess we can check for flang_rt.runtime, flang_rt.runtime.static and flang_rt.runtime.shared targets, then find the output file for them and pass it to the LIT test from CMake, but uniq should probably also work.
There was a problem hiding this comment.
Let me know if #198922 fixes it
It works for me. Thanks!
Summary:
Right now we have a very heavy dependency on
flangfor a single modulefile. The flang build will handle creating the actual module file, we
just need to export the symbols. This PR ports the source file from the
flang .f90 to equivalent C++. This allows us to build
flang-rtstandalone without bringing in the entire flang infrastructure, which is
by far the heaviest build in the entire LLVM project.
I verified the symbols and values against a previous build, so they are
identical.