Page MenuHomePhabricator

[flang][driver]Add experimental flang driver and frontend with help screen
ClosedPublic

Authored by awarzynski on Aug 17 2020, 9:20 AM.

Details

Summary

This is the first patch implementing the new Flang driver as outlined in [1],
[2] & [3]. It created Flang driver (flang-new and Flang frontend driver
(flang-new -fc1) with help screen(-help) and version(--version) options.
These will be renamed as flang and flang -fc1 once the current Flang
throwaway driver, flang, can be replaced

flang-new is implemented in terms of libclangDriver, defaulting the driver
mode to flang (added to libclangDriver in [2]). This ensures that the driver
runs in flang mode regardless of the name of the binary inferred from argv[0].

The design of the new flang compiler and frontend drivers is inspired by it
counterparts in Clang [3]. Currently, the new flang compiler and frontend
drivers re-use Clang libraries: clangBasic, clangDriver and clangFrontend.
To identify Flang options, this patch adds FlangOption enum. printHelp is
updated so that flang-new prints only Flang options.

The new Flang driver is disabled by default. To build it, set
-DBUILD_FLANG_NEW_DRIVER =ON when configuring Flang and LLVM_ENABLE_PROJECTS
with clang.

[1] RFC: new Flang driver - next steps
http://lists.llvm.org/pipermail/flang-dev/2020-July/000470.html
[2] "RFC: Adding a fortran mode to the clang driver for flang"
http://lists.llvm.org/pipermail/cfe-dev/2019-June/062669.html
[3]RFC: refactoring libclangDriver/libclangFrontend to share with Flan
http://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sameeranjoshi added inline comments.Aug 21 2020, 11:24 AM
flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
36

With

clang --version
clang version 11.0.0 (https://github.com/llvm/llvm-project.git 1acf129bcf9a1b51e301a9fef151254ec4c7ec43)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/bin

whereas with

./bin/flang-new --vesion
Flang experimental driver (flang-new)

I see both clang & flang call
llvm::cl::PrintVersionMessage() internally.

Is more information need to be registered in llvm(llvm::cl) for flang to give more detailed output or will that come later once we start adding more patch for driver?

40

The comment in header for ExecuteCompilerInvocation mentions,

	/// \return - True on success.
	bool ExecuteCompilerInvocation(CompilerInstance *Flang);

Do we need to have a false somewhere here?

I see 2 scenarios when ExecuteCompilerInvocation might fail (there could definitely be more) and there we need an indication of failure by returning false,

  1. When there is no actual execution of compiler.
  2. The compilation in not successful.

Address review comments

Replace namespace flang for Fortran::frontend
and fix style

CarolineConcatto marked 17 inline comments as done.Aug 23 2020, 1:55 PM

Hey Folks,
Fist of all, thank you very much for your review.
I have answered some questions and update some of the requests.
There are some that I did not had time to go inside yet.
I have the week of 24th off. IN my return I will tackle all the reviews as:
out of build tree
and some question about the driver

flang/include/flang/Frontend/CompilerInstance.h
12

The way we think is that the driver is split in:
the driver libDriver -> that implements the driver. ATM it is inside clang/lib/Driver. And it can be clang( called inside clang/tools/driver/driver.cpp) or flang-new(called inside flang/tools/driver/driver.cpp) according to the mode the driver is called.
the frontend driver -> it can be:

clang frontend driver: clang -cc1  that calls libclangfrontend 
flang frontend driver: flang-new -fc1 that calls libflangfrontend.

the -xc1 has its functions/methods implemented inside the driver Frontend

So this folder is called Frontend because it belongs to the part that implements the driver frontend of the compiler and we would like to leave this way so it has the same design as clang.

22

I believe I have changed the style of the patch to be flang style.

flang/include/flang/Frontend/FrontendOptions.h
32

This is to not mistake with: enum Format { Source, ModuleMap, Precompiled };

33

Most of these things are going to be set by option::driver. That is the reason it is as it is.

66

Most of these things are going to be set by option::driver. That is the reason it is as it is.

flang/lib/Frontend/CompilerInvocation.cpp
47

Just because the next lines also return DashX.

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
21

Because of CompilerInstance *Flang.

flang/test/lit.cfg.py
40

I like to have the test at a different folder for now so it is clear that the tests inside this folder all belongs to the new driver. So I don't need to open the test to know.
I can implement the requires, but in the future will not need the REQUIRES for the driver test.

SouraVX added inline comments.Aug 23 2020, 10:53 PM
flang/tools/flang-driver/fc1_main.cpp
2

This file-name seems odd considering LLVM style. How about just Flang.cpp or FlangMain.cpp ? or other

@CarolineConcatto - thank you for working on this! @all, thank you for your reviews, this is much appreciated!

While Carol is away, I'll try my best to address all the outstanding comments. I will also update the patch accordingly. I've identified a few small issues myself - that will also be updated in the next patch.

clang/lib/Driver/Driver.cpp
1578

do they need to be considered?

Not at this stage. This is sufficient to make sure that flang-new -help only prints options relevant to Flang.

We may want to fine-tune this in the future, but currently it would be a bit tricky with only 2 options being supported ;-)

flang/include/flang/Frontend/CompilerInstance.h
12

Just to further clarify, I think that it's important to distinguish between:

  • the compiler driver, flang, implemented in terms of libclandDriver (it creates and executes jobs/commands, e.g. linker or frontend jobs)
  • the frontend driver, flang -fc1, implemented in terms of libflangFrontend (it creates frontend actions, e.g. preprocessor actions)

"Frontend" in this case means "Frontend Driver". "Driver" is reserved for the compiler driver, i.e. "libclangDriver".

As Carol mentioned, our terminology is inspired by Clang. Keeping the names consistent felt like a good idea. @tskeith, would you prefer "FrontendDriver" instead of "Frontend"?

(libclangDriver is the Clang compiler driver library that we want to re-use, libflangFrontend is the Flang frontend driver library, inspired by libclangFrontend)

flang/include/flang/Frontend/FrontendOptions.h
32–33

Why isn't the type bool?

@tskeith Bitfields are used here for optimization. An instance of InputKind is created per input file, so this could be a substantial saving (if the bitfields happen to be laid out nicely).

Btw, we won't need fmt_ or preprocessed_ (and the corresponding member methods) just now. I will delete them.

66

Why aren't the types bool?

@tskeith See above.

flang/lib/Frontend/CompilerInvocation.cpp
47

I agree with @tskeith , return InputKind{Language::Unknown}; would be cleaner. I will update this.

92

Not needed for now, I will delete it. Thanks!

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
21

It's not needed, good catch, cheers!

40

@sameeranjoshi These are good points, thank you! However, currently ExecuteCompilerInvocation doesn't do much. When this patch is approved and commited, we'll start adding actual _actions_ (to be executed by this method) that _could_ fail. At that point we will introduce code paths that could lead to return false.

flang/test/lit.cfg.py
40

Let me clarify a bit. I assume that FLANG_BUILD_NEW_DRIVER is Off.

SCENARIO 1: In order to make sure that bin/llvm-lit tools/flang/test/ does not _attempt to run_ the new driver tests, add:
config.excludes.append('Flang-Driver')

With this, the new driver tests will neither be run nor summarized (e.g. as UNSUPPORTED) in the final output.

SCENARIO 2: config.excludes.append('Flang-Driver') will not affect bin/llvm-lit tools/flang/test/Flang-Driver (this time I explicitly specify the Flang-Driver directory). We need:

REQUIERES: new-flang-driver

(or similar) for the new Flang driver tests to be reported as UNSUPPORTED.

I agree with @richard.barton.arm that REQUIRES would be the more canonical approach. We could still leave config.excludes.append('Flang-Driver') there to reduce the noise when running bin/llvm-lit tools/flang/test/. Unless people object, I recommend that we have both.

flang/test/lit.site.cfg.py.in
15
flang/tools/flang-driver/driver.cpp
31

We'd be creating a dedicated header file for no real benefit (this method should not be re-used anywhere else).

flang/tools/flang-driver/fc1_main.cpp
2

This is for consistency with clang: https://github.com/llvm/llvm-project/tree/master/clang/tools/driver. Perhaps Fc1Main.cpp instead?

55

Copy and paste error - cheers for pointing out, fixed in the next revision!

llvm/lib/Option/OptTable.cpp
621 ↗(On Diff #286047)

I agree and have updated this. Thanks for pointing it out!

This part is particularly tricky for Flang. Flang has options with flags that fall both into FlagsToExclude and into FlagsToInclude. For example:

def help : Flag<["-", "--"], "help">, Flags<[CC1Option,CC1AsOption,FlangOption]>,
  HelpText<"Display available options">;

For Flang, before this method is called,

  • CC1Option is added to FlagsToExclude, and
  • FlangOption is added to FlagsToInclude

AFAIK, this is unique for Flang. Other tools never set both FlagsToExclude and FlagsToInclude. As a result, the currently vague relation between FlagsToExclude and FlagsToInclude needs to be clarified. In my opinion, FlagsToInclude should take precedence over FlagsToExclude. That's what I've implemented (that's clarified in the next revision).

andwar updated this revision to Diff 288018.Aug 26 2020, 9:55 AM
andwar added a subscriber: andwar.
  • Canonicalised FLANG_BUILD_NEW_DRIVER in flang/test/CMakeLists.txt and updated the LIT config scripts accordingly
  • Implemented proper handling of -emit-obj (via diagnostics) in CompilerInvocation.cpp
  • Removed members from InputKind which are not needed (and the related methods)
  • Removed/updated comments, added new comments to clarify the design and to highlight the future steps
  • Simplified the original change in OptTable::PrintHelp (made it more generic)
  • Removed unused #includes
  • Fine-tuned the tests (I wanted to make sure that they clearly communicate what is currently supported and what is not)
  • Addressed all the outstanding comments

@andwar is my Phabricator alter-ego. Apologies for the confusion - I had my Arcanist misconfigured.

Added support for out-of-tree builds. It requires -DCLANG_DIR=<llvm-buuld-dir>/lib/cmake/clang when configuring CMake. That's on top of LLVM_DIR and MLIR_DIR.

awarzynski commandeered this revision.Aug 27 2020, 12:20 PM
awarzynski added a reviewer: CarolineConcatto.

@sameeranjoshi Thanks for testing out-of-tree builds. We were missing some minor changes in CMake. I have now uploaded these. Out-of-tree will require -DCLANG_DIR when configuring build while the new driver depends on Clang.

As for your original issue - I suspect that you forgot to update your in-tree directory. This patch needs to be applied both in-tree (in llvm-project) and out-of-tree (flang).

sameeranjoshi accepted this revision.Aug 31 2020, 4:14 AM

Thank you for changes.
I was able to build successfully out-of-tree.
Please update the README.md with the necessary changes.

LGTM!

awarzynski updated this revision to Diff 289111.Sep 1 2020, 2:50 AM
awarzynski marked an inline comment as done.

Update README.md with instructions for building flang-new

richard.barton.arm requested changes to this revision.Sep 2 2020, 11:01 AM

Requesting changes mostly because of the exit status issue on the Driver tests.

A few general questions as well:

  1. Why not implement -### as part of this patch to enable testing more easily?
  2. How come there are no regression tests for -fc1 in flang/test/Frontend? I suppose these come when the first real FrontendAction is implemented?
flang/test/CMakeLists.txt
12

So do the other bools like LINK_WITH_FIR also need this treatment and this is a latent bug? Seems amazing we've not hit that before now.

From an offline conversation ISTR you saying this was to do with how the variable is translated into the lit config? If that is the case then I think there is a function you can use called lit.util.pythonize_bool which converts various strings that mean "True/False" into a real bool for python. That would also let you clean up the lit.cfg.py later on (separate comments).

flang/test/Flang-Driver/driver-error-cc1.c
8

I am surprised that you don't need to prefix this run line with 'not' indicating that flang-new returns 0 exit code in this instance, which seems wrong to me.

flang/test/Flang-Driver/driver-error-cc1.cpp
2

Same comment as above on exit code.

flang/test/Flang-Driver/emit-obj.f90
3

Seems like it would be helpful to have also implemented -### in this patch so that you don't need to write tests like this. You could simply run flang-new -### then check the -fc1 line that would have been emitted for the presence of -emit-obj.

Same comment above regarding exit code.

flang/test/lit.cfg.py
39

This comment should be on line 41

40

Happy with this approach :-)

flang/test/lit.site.cfg.py.in
15

I think it might be better to use lit.util.pythonize_bool to do the canonicalisation. It would let you use config.include_flang_new_driver_test as a real bool later in the file rather than comparing to 1 or 0.

llvm/lib/Option/OptTable.cpp
621 ↗(On Diff #286047)

This change is much better as it is more general and I think it could be acceptable.

I still don't get why you need to have anything set for FlagsToExclude in Flang's case? We only want to display flags that are specifically set as Flang options I think. So we set:

FlagsToInclude=FlangOption
FlagsToExclude=0

And all the options marked as both CC1Option, FlangOption are included because FlangOption is in the include list and all options without FlangOption are skipped.

This will only display all the flags with FlangOption set.

This revision now requires changes to proceed.Sep 2 2020, 11:01 AM

Another random thought that just came to me: what does the new driver do when you invoke it with no input files or options? I could imagine a few sensible outcomes (error: no input (clang and gcc/gfortran behaviour), print version and exit, print help and exit, etc.) and I don't have a strong preference over which we chose here but I think there should be a test for it in test/Driver

awarzynski marked 2 inline comments as done.
  • Adddressed comments from @richard.barton.arm
  • Added FC1Option to ClangFlags (and made other related changes)
  • Added missing code to check return codes from subcommands (flang-new)

Another random thought that just came to me: what does the new driver do when you invoke it with no input files or options? I could imagine a few sensible outcomes (error: no input (clang and gcc/gfortran behaviour), print version and exit, print help and exit, etc.) and I don't have a strong preference over which we chose here but I think there should be a test for it in test/Driver

flang-new inherits generic things like this from libclangDriver. In this case:

error: no input files

This is consistent with clang. I've added a test for this. As for flang-new -fc1 - it in general does very little right now :) clang -cc1 just sits there waiting

flang/test/CMakeLists.txt
12

AFAIK, LINK_WITH_FIR is not used in LIT tests.

I switched to lit.util.pythonize_bool , thanks for the suggestion!

flang/test/Flang-Driver/driver-error-cc1.c
8

Good catch, thanks! Fixed in the latest revision.

flang/test/Flang-Driver/driver-error-cc1.cpp
2

Good catch, thanks! Fixed in the latest revision.

flang/test/Flang-Driver/emit-obj.f90
3

Seems like it would be helpful to have also implemented -### in this patch

As`flang-new` is based on libclangDriver, we get -### for free (i.e. it's already supported).

However, there's a catch: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Driver/Options.td#L369-L370. Currently flang-new --help will not display -### because the the corresponding option definition hasn't been updated (i.e. it is not flagged as a Flang option):

def _HASH_HASH_HASH : Flag<["-"], "###">, Flags<[DriverOption, CoreOption]>,
    HelpText<"Print (but do not run) the commands to run for this compilation">;

We have three options:

  1. Make flang-new --help display options that are flagged as DriverOption or CoreOption. Note this will include many other options (currently unsupported) and extra filtering would be required.
  2. Add FlangOption to the definition of _HASH_HASH_HASH, but IMO that's a bit controversial. Is that what we want to do long-term? Or shall we create a separate category for generic driver options that apply to both Clang and Flang?
  3. Delay the decision until there is more code to base our design on.

I'm in favor of Option 3.

flang/test/lit.cfg.py
39

Updated, ta!

flang/test/lit.site.cfg.py.in
15

I did not know that one - thanks! Updated.

llvm/lib/Option/OptTable.cpp
621 ↗(On Diff #286047)

IIUC, you're assuming that FlagsToInclude != 0 implies that FlagsToExclude == 0. However, once we have an option that's only relevant to Flang we'll start setting both FlagsToInclude and FlagsToExclude (i.e. that assumption will no longer hold).

But this change is needed _just yet_ and hence the confusion. Thanks for pointing out, I will keep it for later!

awarzynski updated this revision to Diff 290435.Sep 8 2020, 2:43 AM

Add missing update in lit.cfg.py

richard.barton.arm accepted this revision.EditedSep 10 2020, 4:14 AM

This LGTM. I think all the previous comments from other reviewers and me have been dealt with so I am happy to accept this revision based on the reviews so far.

I have a few small inline comments to resolve in PrintHelp now we have reverted the functional changes there. Happy to approve on the assumption that they are dealt with and I don't need to see another patch, you can accept it yourself.

I think the non-flang changes to clang and llvm are in-line with what we said in our RFC. I think the summary of these changes are:

  • Don't hard-code the name of the driver in the object constructor, pass it in as an argument + fix up all the clang callsites.
  • Tweak the --help and --version logic to be conditional on driver mode
  • Add some new FlangOption flags to Options.td
  • Change the flang driver binary name to flang-new (in the already approved flang mods)

I am happy to approve these in good faith.

flang/test/Flang-Driver/emit-obj.f90
3

Fair enough. Happy to cross this bridge when we come to it later on. I certainly think that flang-new should support -### one day.

llvm/include/llvm/Option/OptTable.h
246

I don't think this change is correct now that you have reverted the code change to the function.

If I have the same bit set in FlagsToInclude and FlagsToExclude, the FlagsToExclude check fires second and would mean the option is not printed. So FlagsToExclude takes precedence.

Suggest to drop the edit or to correct it.

llvm/lib/Option/OptTable.cpp
613 ↗(On Diff #290435)

With the diff to this logic gone, we should also remove the new newlines so as not to make textual changes unrelated to this patch.

621 ↗(On Diff #286047)

Thanks for the explanation. I agree that it is best to wait for us to have a concrete motivating example before making this tweak to the PrintHelp logic. It will make it easier to test for one thing and will also make the patches more logically consistent.

This revision is now accepted and ready to land.Sep 10 2020, 4:14 AM
awarzynski updated this revision to Diff 291068.EditedSep 10 2020, 12:48 PM
awarzynski marked 2 inline comments as done.

Address final comments + rebase

I also took the liberty and made the following changes in CMake scripts:

  • replaced add_library wiht add_flang_library
  • replaced add_executable with add_flang_tool

These are all NFCs.

This revision was landed with ongoing or failed builds.Sep 11 2020, 3:00 AM
This revision was automatically updated to reflect the committed changes.

We forgot to update the signature and definition of OptTable::findByPrefix: https://github.com/llvm/llvm-project/blob/257b29715bb27b7d9f6c3c40c481b6a4af0b37e5/llvm/include/llvm/Option/OptTable.h#L154-L155. That's required after updating the definition of OptTable::Flags. Not sure none of the compilers complained.

Fixed here: https://github.com/llvm/llvm-project/commit/4eed800b18abaeba3082bf950fbe5c3020c4b592