Page MenuHomePhabricator

[clang][driver] Add basic --driver-mode=flang support for fortran
ClosedPublic

Authored by peterwaller-arm on Jun 20 2019, 8:58 AM.

Details

Summary

As per the RFC in http://lists.llvm.org/pipermail/cfe-dev/2019-June/062669.html.

This patch adds a new Flang mode. When in Flang mode, the driver will
invoke flang for fortran inputs instead of falling back to the GCC
toolchain as it would otherwise do.

The behaviour of other driver modes are left unmodified to preserve
backwards compatibility.

It is intended that a soon to be implemented binary in the flang project
will import libclangDriver and run the clang driver in the new flang
mode.

Please note that since the binary invoked by the driver is under
development, there will no doubt be further tweaks necessary in future
commits.

  • Initial support is added for basic driver phases
    • -E, -fsyntax-only, -emit-llvm -S, -emit-llvm, -S, (none specified)
    • -### tests are added for all of the above
    • This is more than is supported by f18 so far, which will emit errors for those options which are unimplemented.
  • A test is added that ensures that clang gives a reasonable error message if flang is not available in the path (without -###).
  • Test that the driver accepts multiple inputs in --driver-mode=flang.
  • Test that a combination of C and Fortran inputs run both clang and flang in --driver-mode=flang.
  • clang/test/Driver/fortran.f95 is fixed to use the correct fortran comment character.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Include full context.

peterwaller-arm retitled this revision from [DO NOT SUBMIT] [clang][driver] Prototype --driver-mode=fortran support for new flang to [clang][driver] Prototype --driver-mode=fortran support for new flang.Sep 10 2019, 6:49 AM
peterwaller-arm edited the summary of this revision. (Show Details)
peterwaller-arm retitled this revision from [clang][driver] Prototype --driver-mode=fortran support for new flang to [clang][driver] Add basic --driver-mode=fortran support for flang.

I updated this "prototype" revision into the real thing I would like to submit. Please let me know if that was the wrong thing to do and I will resubmit it.

This is an initial implementation which naturally doesn't implement very much. More will come in future patches.

hfinkel added inline comments.Sep 10 2019, 7:40 PM
clang/lib/Driver/ToolChains/Flang.cpp
74

Should this say "Invalid output"?

Fixed assertion message "Input output." => "Invalid output". The erroneous text came was copied from: https://github.com/llvm/llvm-project/blob/6b9df910d04fae62dacc22c1c84f66c0f126cde0/clang/lib/Driver/ToolChains/Clang.cpp#L3849

peterwaller-arm marked an inline comment as done.Sep 11 2019, 12:52 AM

Hi Peter

The overall approach seems good to me and matches how the driver is integrated in the original flang project so not too many surprises. I left a few comments mostly about the scope of the original patch. I wonder how much sense it makes to add support for routing options on to (f18) flang that it does not support yet? Would it not be better to add these options to the clang driver at the same time as they arrive in flang -fc1?

Ta
Rich

clang/lib/Driver/Driver.cpp
4877

This first clause surprised me. Is this a temporary measure or do we never intend to support compiling more than one fortran file at once?

clang/lib/Driver/ToolChains/Flang.cpp
38

F18 does not currently support these options that control the output like -emit-llvm and -emit-obj so this code doesn't do anything sensible at present. Would it not make more sense to add this later on once F18 or llvm/flang grows support for such options?

45

Looks like the F18 option spelling for this is -fparse-only? Or maybe -fdebug-semantics? I know the intention is to throw away the 'throwaway driver' in F18, so perhaps you are preparing to replace this option spelling in the throwaway driver with -fsyntax-only so this would highlight that perhaps adding the code here before the F18 driver is ready to accept it is not the right approach?

68

Similarly to previous comment, do we want to be passing all gfortran options through to F18 in the immediate term or even the long term? I don't think there has been any agreement yet on what the options that F18 will support are (although I agree that gfortran-like options would be most sensible and in keeping with clang's philosophy)

clang/test/Driver/fortran.f95
1–2

... when not in --driver-mode=fortran

peterwaller-arm marked 6 inline comments as done.
peterwaller-arm retitled this revision from [clang][driver] Add basic --driver-mode=fortran support for flang to [clang][driver] Add basic --driver-mode=flang support for fortran.

Updated for review comments and spotted a couple of things myself.

Changes since last time:

  • Removed various options which aren't yet implemented on the f18 side.
  • --driver-mode=fortran is now --driver-mode=flang, which is more consistent with other modes.
  • Added tests which show that multiple inputs (and mixed C+Fortran inputs) are supported by the driver.
  • Updated comments.
  • Removed a branch in getTool which didn't make sense.
  • Updated commit message.
clang/lib/Driver/Driver.cpp
4877

This function answers the question at the scope of a single JobAction. My understanding is that the compiler (as with clang -cc1) will be invoked once per input file.

This does not prevent multiple fortran files from being compiled with a single driver invocation.

clang/lib/Driver/ToolChains/Flang.cpp
38

I've removed them.

45

In the RFC, the intent expressed was to mimick gfortran and clang options. I am making a spelling choice here that I think it should be called -fsyntax-only, which matches those. I intend to make the flang -fc1 side match in the near future.

-fdebug-* could be supported through an -Xflang ... option to pass debug flags through.

68

I have deleted these options pass-throughs. I think you're right in general that we should only add options along with support for those things. The plan is now to add an OPT_flang_Group (or alike) later.

clang/test/Driver/fortran.f95
1–2

Fixed.

Updated comment for IsFlangMode.

Some minor comments about Filetypes and file extensions. Can be ignored or considered for a separate commit.

clang/include/clang/Driver/Driver.h
69

Is the comma by choice?

clang/lib/Driver/ToolChains/Flang.cpp
38

Can it be removed from the Summary of this PR?

clang/lib/Driver/Types.cpp
220

Now that there is a 2018 standard, I am assuming that f18 and F18 are also valid fortran extensions. Can these be added to the File types?

Also, should the TypeInfo file be extended to handle all Fortran file types? ./include/clang/Driver/Types.def

Also, should we capture some information about the standards from the filename extension?

clang/test/Driver/lit.local.cfg
1

For completion .F95 also?
Should we add f03,F03,f08,F08,f18,F18. May be not now.

peterwaller-arm marked 6 inline comments as done.
  • Fixed spurious comma
  • Fixed incorrect comment and changed comment wrapping.

Thanks for the updates. I think this is now looking good and matches the RFC already posted.

One thought that occurs to me when reviewing this. I think we will assume that F18/flang when it lands in the LLVM project will be an optional thing to build and will be an opt-in project at the start that is off by default. What happens when clang --driver-mode=flang is called and flang has not been built? And where would we put a test for that behaviour? If flang were not to be built, should --driver-mode=flang be unsupported/unrecognised and emit an error message?

Might not be something we need to address with this patch, but have you considered this?

clang/include/clang/Driver/Driver.h
186

Need to update the cover letter for the patch then as it still talks about 'fortran mode' rather than 'flang mode'.

clang/lib/Driver/Driver.cpp
4877

Righto - thanks for the explanation.

clang/lib/Driver/ToolChains/Flang.cpp
45

OK - happy with this approach. So -fsyntax-only and -emit-ast will be the new names for f18's -fdebug-semantics and -fdebug-dump-parse-tree I guess.

clang/lib/Driver/Types.cpp
220

Agree with those first two, but that last one is surely a new feature that needs adding when such a feature is enabled in the rest of F18.

We'd need to think that through carefully too. Classic flang never supported such an option and GCC's -std option from C/C++ does not work for Fortran. Also, would that go in the driver or in flang -fc1?

I suggest holding fire on this until we are ready to do it properly.

clang/test/Driver/fortran.f95
2

Need to change "see also --drver-mode=fortran" to "--driver-mode=flang" here.

One thought that occurs to me when reviewing this. I think we will assume that F18/flang when it lands in the LLVM project will be an optional thing to build and will be an opt-in project at the start that is off by default. What happens when clang --driver-mode=flang is called and flang has not been built? And where would we put a test for that behaviour? If flang were not to be built, should --driver-mode=flang be unsupported/unrecognised and emit an error message?

The Clang tests should not actually run flang regardless - they should just test the command line that should be run. An error might be emitted if "flang" isn't found in the relevant search paths, however, that makes sense to me (although we might override this for the purpose of the tests?).

peterwaller-arm marked 14 inline comments as done.
peterwaller-arm edited the summary of this revision. (Show Details)

Thanks everyone for your comments.

Changes since last patch:

  • Reintroduce handling of (no phase arg specified), -S, -emit-llvm, etc.

    The code implementing it was reordered for clarity.

    After internal discussion we concluded it was preferable to land these now even though they are unimplemented on the frontend side, in order to avoid giving assertion errors for unimplemented behaviour.

    It is planned that the frontend will emit sensible errors for unimplemented behaviour.
  • Fixes to the cover letter for --driver-mode=fortran => --driver-mode=flang
  • Add a test (flang-not-installed.f90) which demonstrates the error message if clang is invoked with no flang available, by unsetting PATH. This is a non-"-###" test which also does not depend on the presence of flang.
    • Since it uses shell syntax, it is marked unsupported on windows.
  • In response to a private comment, Flang mode now coerces TY_PP_Fortran to TY_Fortran. This:
    • Leaves the legacy behaviour unchanged
    • Ensures that we do not emit a warning when passing TY_PP_Fortran inputs to -E
    • Should ensure that TY_PP_Fortran is treated exactly as a fortran file
    • Is consistent with f18's intent to not have any notion of preprocessed sources
    • Now the flang.f90 and flang.F90 tests are identical except for the filename
  • Various minor whitespace changes and documentation fixes.
clang/include/clang/Driver/Driver.h
69

It was not. Fixed.

clang/lib/Driver/ToolChains/Flang.cpp
38

They have now been reintroduced after we found that it had an unpleasant failure mode. Better to implement them now, after all, and fail gracefully in the frontend.

45

Agreed. They can still be changed later if necessary.

clang/lib/Driver/Types.cpp
220

My reading of the code is that both [fF]90 and [fF]95 both get turned into TY_Fortran/TY_PP_Fortran.

Both of these in the Types.def end up coerced to "f95", currently. I don't think the driver wants an entry in the Types.def for each fortran standard - it doesn't have them for other languages, for example, and I'm unsure what the benefit of that would be. The main purpose of that table as far as I see is to determine what phases need to run.

My feeling is that the name in the types table should be "fortran". I don't want to make that change in the scope of this PR, since it might be a breaking change. It looks to me like the name works its way out towards human eyeballs but is otherwise unused.

I would like to implement -std inference from the file extension and additional fortran filename extensions (f18 etc) in another change request.

220

Agreed, thanks for clarifying.

I didn't intend to suggest that it was something to be implemented in the near future. Just the future.

clang/test/Driver/fortran.f95
2

Fixed.

clang/test/Driver/lit.local.cfg
1

Since the file I added was .F90, that is what got added.

I have added strictly what was used by the tests under the assumption that others could be introduced when those are used. I would prefer not to add the others until they exist and are exercised (perhaps could happen as part of -std inference).

I think the behaviour for missing flang is fine for now, and I think we can improve on it later on.
We ought to codify (if it is not done already) where flang looks for tools to exec, because I think PATH is probably not the only place it could look to (directory of clang binary, other relative paths, other environment variables and commandline options, etc.) The new test will need revising once we get there.

I think this patch is looking good to be committed - what do you think @hfinkel ?

Hi All, I'm going on leave for two weeks, returning October 14th. I can plausibly respond to comments until 14:00 UTC tomorrow (Thu 25th Sept).

As a quick note, another piece of the puzzle has been submitted for review at https://github.com/flang-compiler/f18/pull/759, which is the flang-side binary which will link to this code and default to running the driver in flang mode.

Friendly ping to everybody watching. I'd like to get this in soon if possible.

Hal - do you think this is close to being accepted? Note that I consider this "the beginning" rather than "the end", since there will be more functionality to add piecewise before this is fully functional. In the meantime, since it is new functionality, it should not break anything.

hfinkel accepted this revision.Oct 22 2019, 9:37 AM

Friendly ping to everybody watching. I'd like to get this in soon if possible.

Hal - do you think this is close to being accepted? Note that I consider this "the beginning" rather than "the end", since there will be more functionality to add piecewise before this is fully functional. In the meantime, since it is new functionality, it should not break anything.

One comment on the test, but otherwise LGTM.

clang/test/Driver/flang/flang-not-installed.f90
11 ↗(On Diff #220856)

I believe that you can write:

REQUIRES: shell

for this.

This revision is now accepted and ready to land.Oct 22 2019, 9:37 AM

I have rebased the patch for conflicts to master and all the tests are passing.

While doing so, I discovered that the test for flang-not-installed was not fit for purpose, because clang actually doesn't first check the PATH, it can find a flang binary which lives in the same directory as itself. I conclude that having such a test is more trouble than it is worth. Adding such a test would involve adding some questionable test-specific machinery. Therefore I've removed that test for now.

I'll leave the patch over the weekend in case anyone objects, otherwise I intend to submit early next week.

peterwaller-arm marked 2 inline comments as done.Oct 24 2019, 8:48 AM
peterwaller-arm added inline comments.
clang/test/Driver/flang/flang-not-installed.f90
11 ↗(On Diff #220856)

I've removed this test for now because I discovered that setting PATH="" is not enough to make clang lose the flang binary, if the flang binary is in the same directory as clang (which will be likely once flang is in-tree).

peterwaller-arm marked an inline comment as done.Oct 29 2019, 4:19 AM

I'm still awaiting commit access, please can someone submit this on my behalf?

Revoking my previous request: I now have commit access and I intend to submit this shortly.

Could we have the clang/test/Driver/flang/flang.F90 and clang/test/Driver/flang/flang.f90 files in different directories please? As macOS's FS is case-insensitive, those two files have the same path from macOS perspective which is causing a whole bunch of issues (including breaking git even with 'core.ignorecase').

Sorry, I had missed the RfC, but it looks like there wasn't a lot of discussion on it anyways.

Adding fortran support to clang's driver has been suggested and decided against before, see "[cfe-commits] [RFC and PATCH] Fortran"

Could we have the clang/test/Driver/flang/flang.F90 and clang/test/Driver/flang/flang.f90 files in different directories please? As macOS's FS is case-insensitive, those two files have the same path from macOS perspective which is causing a whole bunch of issues (including breaking git even with 'core.ignorecase').

One issue is that the test flakily fails about half the time

http://45.33.8.238/mac/2498/summary.html fail
http://45.33.8.238/mac/2499/summary.html pass
http://45.33.8.238/mac/2500/summary.html fail
http://45.33.8.238/mac/2501/summary.html pass
http://45.33.8.238/mac/2502/summary.html fail
http://45.33.8.238/mac/2503/summary.html pass
http://45.33.8.238/mac/2504/summary.html fail

Could we have the clang/test/Driver/flang/flang.F90 and clang/test/Driver/flang/flang.f90 files in different directories please? As macOS's FS is case-insensitive, those two files have the same path from macOS perspective which is causing a whole bunch of issues (including breaking git even with 'core.ignorecase').

This was fixed by Jeremy Morse in 6c0a160c2d33e54aecf1538bf7c85d8da92051be - thanks.

I'm investigating the non-deterministic failure but don't have access to a mac to test.

@thakis is the flaky test resolved by 6c0a160c or is it still outstanding?

6c0a160c2d33e54aecf1538bf7c85d8da92051be renamed the file (thanks Jeremy).

Could we have the clang/test/Driver/flang/flang.F90 and clang/test/Driver/flang/flang.f90 files in different directories please? As macOS's FS is case-insensitive, those two files have the same path from macOS perspective which is causing a whole bunch of issues (including breaking git even with 'core.ignorecase').

This was fixed by Jeremy Morse in 6c0a160c2d33e54aecf1538bf7c85d8da92051be - thanks.

I'm investigating the non-deterministic failure but don't have access to a mac to test.

@thakis is the flaky test resolved by 6c0a160c or is it still outstanding?

Green dragon survived the change (because it doesn't run git pull I assume) and is running the tests soon-ish: http://green.lab.llvm.org/green/view/Clang/job/clang-stage1-cmake-RA-incremental/4685/console

Thanks for chiming in.

Sorry, I had missed the RfC, but it looks like there wasn't a lot of discussion on it anyways.

Apologies @thakis - did I jump the gun? And if so, what could I have done differently?

Adding fortran support to clang's driver has been suggested and decided against before, see "[cfe-commits] [RFC and PATCH] Fortran"

Interesting, I had missed that conversation! Google and DDG fail to find it for me even quoting sentences out of it. Found it by wgetting the gzip mailing list archives and grepping them - is there a better way? :)

That conversation was in 2012, and since then flang has been accepted as a subproject to LLVM in "[llvm-dev] f18 is accepted as part of LLVM project!". Since the acceptance, the project has renamed to flang, and is in the process of actively being integrated into the llvm project.

As discussed in the RFC, I understand that libclangDriver was always intended to be a flexible driver used by other frontends, so in light of that, does the change still seem unreasonable? My expectation is that the footprint of fortran on libclangDriver should only be quite modest.

Thanks for chiming in.

Sorry, I had missed the RfC, but it looks like there wasn't a lot of discussion on it anyways.

Apologies @thakis - did I jump the gun? And if so, what could I have done differently?

Whoops, sorry! I had meant to delete that bit. I started writing that, then went back to the old discussion thread, and saw that there was actually agreement on giving clang's driver knowledge about fortran (but not the preprocessor etc). So this is all good as far as I can tell. Please ignore that part.

The test is still failing fairly consistently on my bot, even after 6c0a160c . (And that change required me to change my bots from git merge --ff-only origin/master to git reset --hard origin/master after fetching, but that's a good change to do anyways ;) ). Does http://45.33.8.238/mac/2528/step_6.txt make any sense to you, or should I ssh in and debug?

Interesting, I had missed that conversation! Google and DDG fail to find it for me even quoting sentences out of it. Found it by wgetting the gzip mailing list archives and grepping them - is there a better way? :)

(If you want, I can forward you the thread. But as I said, I just failed to delete my toplevel comment; all good.)

@thakis: I found the thread at https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20120430/057199.html

@thakis: I can't make quick sense of the failure from those logs alone. I would appreciate it very much to see the output of the clang -### run lines to see what's missing. It should be showing that it would invoke flang.

@thakis: I can't make quick sense of the failure from those logs alone. I would appreciate it very much to see the output of the clang -### run lines to see what's missing. It should be showing that it would invoke flang.

It's the very first run line that's failing. The output looks like so:

$ out/gn/bin/clang --driver-mode=flang -### -E /Users/thakis/src/llvm-project/clang/test/Driver/flang/flang.f90
clang version 10.0.0 
Target: x86_64-apple-darwin18.7.0
Thread model: posix
InstalledDir: /Users/thakis/src/llvm-project/out/gn/bin
clang: warning: /Users/thakis/src/llvm-project/clang/test/Driver/flang/flang.f90: previously preprocessed input [-Wunused-command-line-argument]

I've thought about it for a moment and I'm currently at a loss to quickly explain why this would only fail on darwin. In my patch, the change to LookupTypeForExtension should prevent clang from reaching this state where it complains about a preprocessed input.

I don't have access to a darwin machine right now to dive in, understand and fix it.

I propose to mark the test UNSUPPORTED: darwin until more is understood about the breakage. I assume there is no issue with that?

@thakis: I've marked the test unsupported in c75cd3c7f0f. Hopefully that makes your builder happy! I'll figure out what is going on and fix it.

Thanks!

Looks like it's just the -E test that's causing problems; the test passes with that disabled:

$ git diff
diff --git a/clang/test/Driver/flang/flang.f90 b/clang/test/Driver/flang/flang.f90
index 97e4847f843..4cbc2cd8754 100644
--- a/clang/test/Driver/flang/flang.f90
+++ b/clang/test/Driver/flang/flang.f90
@@ -1,6 +1,5 @@
 ! D63607 made mac builders unhappy by failing this test, and it isn't
 ! yet obvious why. Mark as unsupported as a temporary measure.
-! UNSUPPORTED: darwin
 
 ! Check that flang -fc1 is invoked when in --driver-mode=flang.
 
@@ -21,10 +20,9 @@
 
 ! Check that f90 files are not treated as "previously preprocessed"
 ! ... in --driver-mode=flang.
-! RUN: %clang --driver-mode=flang -### -E                  %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-E %s
+! RUN: %clang --driver-mode=flang -###                   %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-E %s
 ! CHECK-E-NOT: previously preprocessed input
-! CHECK-E-DAG: "-E"
-! CHECK-E-DAG: "-o" "-"
+! CHECK-E-DAG: "-o"
 
 ! RUN: %clang --driver-mode=flang -### -emit-ast           %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-AST %s
 ! CHECK-EMIT-AST-DAG: "-triple"

I'll debug a bit more.

clang/lib/Driver/Driver.cpp
4883

(please run clang-format on your new code -- it might also answer why some enums have a comma after the last entry ;) )

I had some time to take a look. D69636 makes things work on mac.

Sadly I'm just noticing this:

I don't really agree with the rationale for adding fortran support to the clang driver. My proposed design here would be to move the aspects you need out of clang and into a separate library in llvm that you can use instead.

I don't necessarily want to remove this immediately, but I think work should be done to move forward in this other direction and eventually migrate this out.

Thanks!