Page MenuHomePhabricator

[Flang][Driver]Add new flang driver main() entrypoint
Needs ReviewPublic

Authored by CarolineConcatto on Apr 29 2020, 7:36 AM.

Details

Summary

Depends on D73951

This work implements the "driver" part of an RFC sent to cfe-dev in [1].
The entry point adds a binary called bin/flang-tmp because ATM flang is in use
by flang.sh.
The goal is to have the old flang.sh removed from the install to make way
for flang-tmp.

bin/flang-tmp is implemented in terms of clang::Driver, defaulting to the
--driver-mode=flang added to clang in [2]. Therefore it adds libClang in
the CMake.

This patch uses a new flag ('fortran-fe' for fortran frontend) introduced
in [3], which is under review. bin/flang-tmp will select the frontend
flang-tmp -fc1 instead of flang -fc1 (in the code we set -fortran-fe=flang-tmp)
while flang is at use.

Regression tests are added in test/driver, which mirror some tests in the
clang/test/Driver directory, flang.f90 and flang.F90. The primary difference is
that the clang tests run clang --driver-mode=flang <input>, whereas these tests
run flang <input>.

[1] "RFC: Adding a fortran mode to the clang driver for flang"
http://lists.llvm.org/pipermail/cfe-dev/2019-June/062669.html
[2] [clang][driver] Add basic --driver-mode=flang support for fortran
https://reviews.llvm.org/D63607
[3][Clang][Driver]Add logic to search for flang frontend
https://reviews.llvm.org/D73951

Signed-off-by: Caroline Concatto <caroline.concatto@arm.com>

Diff Detail

Event Timeline

CarolineConcatto marked an inline comment as done.

Minor fixes on CMakeLists.txt

ChinouneMehdi added a project: Restricted Project.Apr 30 2020, 8:26 AM

This change adds a build dependency on clang.

What's the level-of-effort required to extract the shared functionality into llvm/lib/Frontend?

What's the level-of-effort required to extract the shared functionality into llvm/lib/Frontend?

@sscalpone , let me chime in. This patch only really requires clang::DiagnosticsEngine (from clangBasic) and clang::driver (from clangDriver), so not that much. Extracting thess into a separate library shouldn't be too difficult on it's own. However, it would mean redefining the libraries in clang.

  • This is likely to have a huge impact on a lot of mature upstream and downstream LLVM projects (these projects would have to re-define their dependencies)
  • In my opinion this should be discussed with the community first (i.e. via llvm-dev, cfe-dev and flang-dev)
  • It could have bigger impact on LLVM/clang that I don't see right now.

Also, as we make progress we are likely to discover that we need to repeat this exercise a few more times (i.e. refactor more clang components).

We wanted to take an alternative approach - develop a clang-based replacement for flang (the bash wrapper) and only then refactor the common bits into a dedicated library. I think that at that point we will be better informed as what exactly is required and truly _common_.

@sscalpone can you tell us what is your concern? It is compiling time now that the driver is not fully functional?

If it is compiling time, what I can do to save compiler time while the driver is not functional is do the same as FIR by using a similar CMake variable as LINK_WITH_FIR.

I could create a variable for the driver, that we would only build it with clang dependencies when we want to build the new driver.

Now, if the problem is linking the driver with Clang than I agree with @awarzynski and this is a problem that we may be able to solve later in this project.

flang/CMakeLists.txt
246

This is a mistake. The patch works with -Werror. I'll revert this

I think @sscalpone's point is that the following currently works but will not work after this patch since we need to build clang (add to DLLVM_ENABLE_PROJECTS) also.
cmake ../llvm -DLLVM_ENABLE_PROJECTS="mlir;flang"

Ok, when the driver is fully implemented this could be true.
But for now if this is the problem I can do the same as MLIR for FIR.

Ok, when the driver is fully implemented this could be true.
But for now if this is the problem I can do the same as MLIR for FIR.

I would advise against making sections of the compiler build optional. It leads to mischief.

@schweitz Ok, I am open to solutions for the concern @sscalpone brought.
I suggested the flag because I can see it is what is done with MLIR for FIR.
ATM it is not possible to remove clang from the libraries because the Driver class is inside clang.
Probably the Driver class will need to be refactored in the future, maybe when we have a stable flang driver.
But for now, it will only delay more flang driver functionality.

Is it fine to always build clang and the driver as a standard?

tskeith added a subscriber: tskeith.May 4 2020, 7:01 AM

I like the idea of a cmake option as a temporary way to disable building the driver until the clang part can be properly extracted out. One shouldn't be forced to build all of clang just to build flang.

Extracting these into a separate library shouldn't be too difficult

@CarolineConcatto @awarzynski llvm/lib/Frontend was created exactly for this kind of work. I recommend that you plan to refactor the components that are required & share your proposal with cfe-dev and llvm-dev. The long-range plan shared with cfe-dev [1] may need to be updated.

[1] http://lists.llvm.org/pipermail/cfe-dev/2019-June/062669.html

@schweitz Ok, I am open to solutions for the concern @sscalpone brought.
I suggested the flag because I can see it is what is done with MLIR for FIR.
ATM it is not possible to remove clang from the libraries because the Driver class is inside clang.
Probably the Driver class will need to be refactored in the future, maybe when we have a stable flang driver.
But for now, it will only delay more flang driver functionality.

My point is that if you make it optional, others may understand that to mean it is not a first-class part of the compiler. You risk having changes that simply do not consider it. I believe "optional FIR" was a mistake and it should have been removed.

Thank you all for your inputs!
This discussion has raised some valuable points and we’d like to address all of them.
Thank you for the suggestion @sscalpone! Yes, we will send and updated RFC shortly and continue the discussion on the mailing list.
The older RFC sent by Peter Waller was done before flang landed in llvm-project monorepo and needs some re-thinking and pruning.