Page MenuHomePhabricator

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

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

Note: In case you see this early, the email isn't yet sent to the list. I'll link it here when it is, likely tomorrow.

Even though I know you didn't send the email yet, can you please upload the diff with full context? Thanks :-)

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.Tue, Sep 10, 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.Tue, Sep 10, 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.Wed, Sep 11, 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
4749

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 ↗(On Diff #219666)

... 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
4749

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 ↗(On Diff #219666)

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
70

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
195

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 ↗(On Diff #220646)

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
189

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
4749

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
195

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 ↗(On Diff #220681)

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
70

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
195

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.

195

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 ↗(On Diff #220681)

Fixed.

clang/test/Driver/lit.local.cfg
1 ↗(On Diff #220646)

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).