Page MenuHomePhabricator

[flang][driver] Delete `f18` (i.e. the old Flang driver)
ClosedPublic

Authored by awarzynski on Jul 12 2021, 6:20 AM.

Details

Summary

This patch removes f18, a.k.a. the old driver. It is being replaced
with the new driver, flang-new, which has reached feature parity with
f18 a while ago. This was discussed in [1] and also in [2].

With this change, FLANG_BUILD_NEW_DRIVER is no longer needed is deleted
as well. This means that we are making the dependency on Clang permanent
(i.e. it cannot be disabled with a CMake flag).

LIT set-up is updated accordingly. All references to f18 or f18.cpp
are either updated or removed.

[1] https://lists.llvm.org/pipermail/flang-dev/2021-June/000742.html
[2] https://reviews.llvm.org/D103177

Diff Detail

Event Timeline

awarzynski created this revision.Jul 12 2021, 6:20 AM
awarzynski requested review of this revision.Jul 12 2021, 6:20 AM
Herald added a project: Restricted Project. · View Herald Transcript

I get some build errors like the following with this patch.

c++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/flang/lib/Frontend -I/home/kircha02/llvm-project/flang/lib/Frontend -I/home/kircha02/llvm-project/flang/include -Itools/flang/include -Iinclude -I/home/kircha02/llvm-project/llvm/include -isystem /home/kircha02/llvm-project/llvm/../mlir/include -isystem tools/mlir/include -isystem tools/clang/include -isystem /home/kircha02/llvm-project/llvm/../clang/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-copy -fno-strict-aliasing -fno-semantic-interposition -O3 -DNDEBUG  -fno-exceptions -fno-rtti -std=c++17 -MD -MT tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o -MF tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o.d -o tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o -c /home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp
/home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp: In function 'bool ParseFrontendArgs(Fortran::frontend::FrontendOptions&, llvm::opt::ArgList&, clang::DiagnosticsEngine&)':
/home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp:105:36: error: 'class clang::DiagnosticsEngine' has no member named 'getNumErrors'; did you mean 'unsigned int clang::DiagnosticsEngine::NumErrors'? (not accessible from this context)
  105 |   unsigned numErrorsBefore = diags.getNumErrors();

Build steps used,

cmake -G Ninja ../llvm -DLLVM_ENABLE_PROJECTS="flang;clang;mlir;openmp" -DLLVM_TARGETS_TO_BUILD=AArch64 -DCMAKE_BUILD_TYPE=Release
nice -n 10 ninja

Since @sscalpone is the original author of the throw-away driver. It would be great to get his opinion before deleting f18.cpp. @klausler expressed some reservations in removing f18.cpp but subsequently said it is OK to delete it in the llvm-project/flang repo.

Regarding the dependency on Clang:

  1. AMD engineers raised some issues in slack regarding build time in flang-compiler slack. Could @kiranktp let us know whether they have issues or whether they are OK?
  2. I am guessing the default assumption is that the dependency on Clang will be permanent and there are no plans to remove the dependency.
  3. One of the reasons for making the dependency on Clang permanent is some requirement for OpenMP that @jdoerfert mentioned in the call. I am personally not aware of any dependencies as of now for OpenMP on clang. I am assuming that this has something to do with offloading. Will this cause MLIR to have a dependency on Clang too (which might run into issues with the MLIR team)? Or is this again a driver induced dependency?
flang/include/flang/Frontend/CompilerInvocation.h
187

spelling: predefined

flang/test/lit.cfg.py
43

Is this required?

I get some build errors like the following with this patch.

c++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/flang/lib/Frontend -I/home/kircha02/llvm-project/flang/lib/Frontend -I/home/kircha02/llvm-project/flang/include -Itools/flang/include -Iinclude -I/home/kircha02/llvm-project/llvm/include -isystem /home/kircha02/llvm-project/llvm/../mlir/include -isystem tools/mlir/include -isystem tools/clang/include -isystem /home/kircha02/llvm-project/llvm/../clang/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-copy -fno-strict-aliasing -fno-semantic-interposition -O3 -DNDEBUG  -fno-exceptions -fno-rtti -std=c++17 -MD -MT tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o -MF tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o.d -o tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o -c /home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp
/home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp: In function 'bool ParseFrontendArgs(Fortran::frontend::FrontendOptions&, llvm::opt::ArgList&, clang::DiagnosticsEngine&)':
/home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp:105:36: error: 'class clang::DiagnosticsEngine' has no member named 'getNumErrors'; did you mean 'unsigned int clang::DiagnosticsEngine::NumErrors'? (not accessible from this context)
  105 |   unsigned numErrorsBefore = diags.getNumErrors();

Build steps used,

cmake -G Ninja ../llvm -DLLVM_ENABLE_PROJECTS="flang;clang;mlir;openmp" -DLLVM_TARGETS_TO_BUILD=AArch64 -DCMAKE_BUILD_TYPE=Release
nice -n 10 ninja

@kiranchandramohan I am not able to reproduce this :/ Also, the pre-merge CI seems fine and this patch does not modify flang/lib/Frontend/CompilerInvocation.cpp. Very odd! The error message seems incorrect though:

Perhaps there was another error earlier?

  1. I am guessing the default assumption is that the dependency on Clang will be permanent and there are no plans to remove the dependency.

No. Lifting clangDriver would be good for both Flang and Clang, but it will require a non-trivial amount of effort. It's been deprioritised in favour of other work items that are likely to bring comparable benefits at a lower cost (e.g. code-gen support).

  1. Will this cause MLIR to have a dependency on Clang too (which might run into issues with the MLIR team)? Or is this again a driver induced dependency?

I am not too familiar with Flang's dependency on MLIR, but I don't see why Flang's dependency on Clang would make MLIR require Clang to build. As long as MLIR does not require Flang to build, all should be fine :) Perhaps I misunderstood your question?

I get some build errors like the following with this patch.

c++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/flang/lib/Frontend -I/home/kircha02/llvm-project/flang/lib/Frontend -I/home/kircha02/llvm-project/flang/include -Itools/flang/include -Iinclude -I/home/kircha02/llvm-project/llvm/include -isystem /home/kircha02/llvm-project/llvm/../mlir/include -isystem tools/mlir/include -isystem tools/clang/include -isystem /home/kircha02/llvm-project/llvm/../clang/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-copy -fno-strict-aliasing -fno-semantic-interposition -O3 -DNDEBUG  -fno-exceptions -fno-rtti -std=c++17 -MD -MT tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o -MF tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o.d -o tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o -c /home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp
/home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp: In function 'bool ParseFrontendArgs(Fortran::frontend::FrontendOptions&, llvm::opt::ArgList&, clang::DiagnosticsEngine&)':
/home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp:105:36: error: 'class clang::DiagnosticsEngine' has no member named 'getNumErrors'; did you mean 'unsigned int clang::DiagnosticsEngine::NumErrors'? (not accessible from this context)
  105 |   unsigned numErrorsBefore = diags.getNumErrors();

Build steps used,

cmake -G Ninja ../llvm -DLLVM_ENABLE_PROJECTS="flang;clang;mlir;openmp" -DLLVM_TARGETS_TO_BUILD=AArch64 -DCMAKE_BUILD_TYPE=Release
nice -n 10 ninja

@kiranchandramohan I am not able to reproduce this :/ Also, the pre-merge CI seems fine and this patch does not modify flang/lib/Frontend/CompilerInvocation.cpp. Very odd! The error message seems incorrect though:

Perhaps there was another error earlier?

Works fine on a fresh build.

  1. I am guessing the default assumption is that the dependency on Clang will be permanent and there are no plans to remove the dependency.

No. Lifting clangDriver would be good for both Flang and Clang, but it will require a non-trivial amount of effort. It's been deprioritised in favour of other work items that are likely to bring comparable benefits at a lower cost (e.g. code-gen support).

I am guessing that upstreaming flang, feature-parity with classic-flang (Fortran + OpenMP) and productisation of llvm/flang are always going to be more important than removing the dependency. So the dependency is likely to stay.

  1. Will this cause MLIR to have a dependency on Clang too (which might run into issues with the MLIR team)? Or is this again a driver induced dependency?

I am not too familiar with Flang's dependency on MLIR, but I don't see why Flang's dependency on Clang would make MLIR require Clang to build. As long as MLIR does not require Flang to build, all should be fine :) Perhaps I misunderstood your question?

Flang and MLIR have OpenMP support through the OpenMP dialect. If for OpenMP based offloading, flang has a dependency on clang then it might be the case that MLIR has as well.

Meinersbur added inline comments.Jul 22 2021, 2:58 PM
flang/docs/Overview.md
41

Shouldn't the docs refer to flang instead of flang-new? AFAIU flang will eventually be replaced by a proper executable that does not call gfortran anymore and flang-new go away.

flang/tools/f18/flang
185
247–258

Is F18_FC still defined?

336–337
llvm/CMakeLists.txt
84–85
awarzynski marked 3 inline comments as done.Jul 23 2021, 7:46 AM
awarzynski added inline comments.
flang/docs/Overview.md
41

I think that this document should only refer to the frontend driver rather than the compiler driver. In our case, that's either f18 (compiler driver + frontend driver) or flang-new -fc1 (new frontend driver). In fact, some options referred here are only available in flang-new -fc1 (i..e. these options are frontend-specific).

Currently, flang is a yet another tool - it's neither a compiler nor a frontend driver, so I don't want to use it here. IMO, we should first rename it as e.g. flang-to-gfortran (or something similar) to clarify what it actually does. Then, we can claim flang for the actual driver and update all the docs accordingly.

flang/test/lit.cfg.py
43

Some tests use it, e.g.: https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/missing-arg.f90#L4

Technically speaking, this patch makes it irrelevant. However, I wanted to keep this change relatively small and easy to review. If I do remove new-flang-driver, I will have to update further 20 files. I'm fine with that, but I can also remove it in a subsequent NFC patch. What do you recommend?

flang/tools/f18/flang
247–258

Only here: https://github.com/llvm/llvm-project/blob/main/flang/docs/ReleaseNotes.md#using-flang. I think that we do want to support compilers other than gfortran (so we need to keep this variable).

336–337

This last sentence only applies to f18. I am deleting it.

Meinersbur added inline comments.Jul 23 2021, 12:44 PM
flang/docs/Overview.md
41

Indeed this document is in the "Design Document" section of https://flang.llvm.org/docs/. However, could you still add a note that this is developer, not end-user documentation. Seems that we don't have and-user documentation yet.

If we follow the analogy of clang, then the eventual goal is that flang would be the primary user command and gfortran-compatible wrapper. flang -fc1 An implementation detail, but still accessible with -Xflang. Where does flang-new fit in here?

flang/tools/f18/flang
247–258

Maybe flang-new should have used FLANG_FC instead, but we can keep it as a (temporary?) compatibility option.

Mark Overview.md as developer rather than end-user doc. Replace F18_FC with FLANG_FC.

awarzynski added inline comments.Jul 30 2021, 7:13 AM
flang/docs/Overview.md
41

I'll update the documentation shortly.

As for flang vs flang-new vs flang -fc1, the plan is to first rename flang as e.g. flang-to-gfortran. We still need to decide when to do this. Once flang is renamed, flang-new can be renamed as flang and flang-new -fc1 becomes flang -fc1 (we don't really have flang -fc1 right now).

flang-new is a rather confusing and an enigmatic name. One big benefit of renaming flang (the bash script) sooner rather than later is that flang-new would become flang and the weird name would be gone.

flang/tools/f18/flang
247–258

Good point, updated.

Meinersbur added inline comments.Aug 4 2021, 8:34 AM
flang/docs/Overview.md
41

I'll update the documentation shortly.

Still working on this?

This revision is now accepted and ready to land.Aug 4 2021, 8:46 AM
awarzynski added inline comments.Aug 4 2021, 9:19 AM
flang/docs/Overview.md
41

Are you referring to flang-new -fc1 here? I would like to keep it for now. This document is intended for developers, so I'm hoping that that's fine.

flang (the bash script) would need some extra logic for flang -fc1 to work as expected. We could add it, but would it be worth it? I'd rather limit the script to the required minimum.

Meinersbur added inline comments.Aug 4 2021, 2:11 PM
flang/docs/Overview.md
41

It just looked like you were planning to add more documentation (that you want me to review). I think that state as it is is fine.

awarzynski added inline comments.Aug 5 2021, 2:25 AM
flang/docs/Overview.md
41

I got distracted and then forgot about it, sorry. So, I would like to merge this change as is. For extra documentation, please see https://reviews.llvm.org/D107543. We can continue the discussion there.

kiranchandramohan accepted this revision.Aug 5 2021, 2:52 AM

AFAIU there have been no objections to merging this patch either in the review here or in the calls.

With this patch, we will have a dependency on Clang. Removing the dependency requires some large scale refactoring in Clang. As @awarzynski says, at the moment, enabling code generation is more important so we will not immediately pursue refactoring. Removing the dependency on Clang is captured in the ticket below.
https://bugs.llvm.org/show_bug.cgi?id=50185

Future dependencies on Clang can come while implementing OpenMP offloading (https://clang.llvm.org/docs/ClangOffloadBundler.html). We should try to avoid new dependencies by refactoring or if that is not possible only have the dependency when the offloading feature is enabled.

I am accepting this path with a promise to spend some time to help remove the dependency on Clang.

mehdi_amini added inline comments.
llvm/CMakeLists.txt
86

A few lines above, we enable MLIR automatically as a dependency of clang instead of erroring out, any reason you didn't just apply the same logic here?

awarzynski added inline comments.Mon, Sep 6, 2:13 AM
llvm/CMakeLists.txt
86

This came up before. LLVM_ENABLE_PROJECTS is a CACHE variable (i.e. user variable), so we shouldn't modify it here. In https://reviews.llvm.org/D101842, @Meinersbur suggested tweaking LLVM_TOOL_*_BUILD instead. See also this comment.

The lack of consistency in how the MLIR and CLang dependencies are treated is not great, but I've not had the bandwidth to fix this. Do you have a preferred approach here? I'm not a CMake expert - suggestions are very welcome! Btw, apologies for the delay, I was AFK for a couple of weeks.

Meinersbur added inline comments.Tue, Sep 7, 12:17 PM
llvm/CMakeLists.txt
86

To reiterate, CACHE variables are considered user-controlled and IMHO should not be modified by the script. Interaction is complicated and the CMake manual warns at several places. What may happen after the initial set(LLVM_ENABLE_PROJECTS CACHE):

  • set(LLVM_ENABLE_PROJECTS): Creates a new variable in the current scope that shadows the CACHE variable. When leaving the scope, the CACHE variable becomes visible again. Note that lines 75 and 81 do exactly this.
  • set(LLVM_ENABLE_PROJECTS CACHE): Ignored, but may or may not remove any shadowing local scope variable again, such as those set at lines 75 and 81 (see change in CMake 3.21)
  • set(LLVM_ENABLE_PROJECTS CACHE FORCE): Overwrites the user-set CACHE variable (probably what you expected to happen), possibly also removing the local scope variable.

Re-setting a cached variable may also cause the cmake configure script to run again at make after the initial run. Now the value of the cached value has the overwritten value from the beginning, which may change script behaviour before it was overwritten.

I suggested to make better use of LLVM_TOOL_*_BUILD instead, whose default value currently is just derived from LLVM_ENABLE_PROJECTS, but could also take dependencies into account.