Page MenuHomePhabricator

[flang][driver] Add support for `-fsyntax-only`
ClosedPublic

Authored by awarzynski on Dec 8 2020, 8:36 AM.

Details

Summary

The behaviour triggered with this flag is consistent with -fparse-only in flang (i.e. the throwaway driver). This new spelling is consistent with Clang and gfortran, and was proposed and agreed on for the new driver in [1].

[1] http://lists.llvm.org/pipermail/flang-dev/2020-November/000588.html

Diff Detail

Event Timeline

awarzynski created this revision.Dec 8 2020, 8:36 AM
awarzynski requested review of this revision.Dec 8 2020, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 8:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
awarzynski added a project: Restricted Project.

A few comments.

clang/lib/Driver/ToolChains/Flang.cpp
43–44 ↗(On Diff #310234)

Why?

flang/include/flang/Frontend/CompilerInstance.h
32

Nit: Semaantics -> Semantics

103–107

What is the coding style of this file?

flang/lib/Frontend/CMakeLists.txt
16

Is Evaluate needed?

flang/lib/Frontend/FrontendActions.cpp
82

What is the use of output stream here?

83

Will the Prescan step have happened before?

85

Nit: semantincs -> semantics

flang/test/Flang-Driver/syntax-only.f90
2

should there be a test without fc1?

Thanks for patch, a few queries below.

Are the tests in clang which are related to flang failing at https://reviews.llvm.org/B81474?

flang/include/flang/Frontend/FrontendActions.h
28

Do you think following a pattern here for mapping with enum ActionKind would be better?
something like ParseSyntaxOnlyAction ?

flang/include/flang/Frontend/FrontendOptions.h
1

C++ ?

flang/lib/Frontend/FrontendActions.cpp
97

What happens in case one of the stages in the pipeline fails?
Where is the error recovery done for e.g if parsing fails ?

flang/unittests/Frontend/PrintPreprocessedTest.cpp
121

Execute

awarzynski marked 4 inline comments as done.Dec 10 2020, 9:32 AM

Thank you for your reviews! I'll submit an updated patch shortly.

clang/lib/Driver/ToolChains/Flang.cpp
43–44 ↗(On Diff #310234)

These 2 lines mean that flang-new -fsyntax-only is translated to flang-new -fc1 -fsyntax-only -triple <triple>. However,

$ bin/flang-new -fc1 -triple
error: unknown argument: '-triple'

i.e. the compiler frontend driver, flang-new -fc1, doesn't support this flag.

I extracted this change here: https://reviews.llvm.org/D93027. Also updated the tests that rely on this. Thanks for pointing those failures @sameeranjoshi!

flang/include/flang/Frontend/CompilerInstance.h
103–107
flang/include/flang/Frontend/FrontendActions.h
28

Yes, that would be better, thanks!

flang/lib/Frontend/CMakeLists.txt
16

Don't think so. I built with -DBUILD_SHARED_LIBS=On, which usually is pretty good at catching missing CMake dependencies.

flang/lib/Frontend/FrontendActions.cpp
82
83
97

That's a great catch, thank you!

flang/test/Flang-Driver/syntax-only.f90
2

This is a tricky question :) The answer depends on what we want to test here:

  • fronted driver (flang-new -fc1),
  • compiler driver (flang-new),
  • Flang frontend, or
  • libclangDriver?

In general flang-new -fsyntax-only != flang-new -fc1 -fsyntax-only. Admittedly, currently these are actually equal. However, if you compare clang -### -fsyntax-only and clang -cc1 -fsyntax-only then you will see a rather huge difference. flang-new is also heading in that direction.

To me -fsyntax-only is actually a frontend driver flag. If we also test flang-new -fsyntax-only then that triggers quite a few code-paths within libclangDriver. But this patch doesn't really touch that logic. Instead, it adds some logic in the frontend driver and hence IMO it should be testing the frontend driver only.

But one can argue that from user's perspective:

  • flang-new -fc1 -fsyntax-only input.f, and
  • flang-new input.f

should be identical. But then again, are we verifying that the user gets what they expect or the internal implementation?

Separately, the fact that libclangDriver _forwards_ -fsyntax-only to flang-new -fc1 is tested here: https://github.com/llvm/llvm-project/blob/1365718778b4ce05587afa22835282c5d3f835b7/clang/test/Driver/flang/flang.f90#L16. (it's a bit more nuanced, that option is not really forwarded, but that's an implementation detail).

Update according to PR comments

  • extracted the -triple related part into https://reviews.llvm.org/D93027
  • made sure that flang-new and flang-new -fc1 return error when semantic checks fail
  • fixed typos

Fine-tune the output for consistency with flang/f18

This is just a minor change to make sure that:

  • flang-new -fc1 -fsyntax-only, and
  • f18 -fparse-only

are as similar as possible.

awarzynski edited the summary of this revision. (Show Details)Dec 11 2020, 8:32 AM
awarzynski added reviewers: kiranktp, AMDChirag.
sameeranjoshi accepted this revision.Dec 13 2020, 11:11 PM

Looks good from my end.
Thank you.

This revision is now accepted and ready to land.Dec 13 2020, 11:11 PM
CarolineConcatto added inline comments.
flang/lib/Frontend/FrontendActions.cpp
88

Just a quick question:
Why you cannot use this function:
CompilerInvocation::SetDefaultFortranOpts
instead of using semanticsContext.set_moduleDirectory("."s);
?

awarzynski added inline comments.Dec 15 2020, 8:44 AM
flang/lib/Frontend/FrontendActions.cpp
88

Hard-coded values are bad, thanks for pointing that out!

We can't use CompilerInvocation::SetDefaultFortranOpts here, as that's for Fortran::parser::Options, and here we are dealing with the internal state of Fortran::semantics::SemanticsContext.

However, moduleDirectory_ is in fact initialised to "."s by default: https://github.com/llvm/llvm-project/blob/8acb5f2723ecaf0f1904a085ad79d0623cec38f6/flang/include/flang/Semantics/semantics.h#L185.

So, it's safe to delete this line for now. A proper solution/approach will be implemented once we add support for -module-dir: https://docs.google.com/spreadsheets/d/1JRe39lP_KhtkYxFEIvwrCFlE5v1Ofa_krOHI-XXXWPY/edit#gid=0.

Address comment from @CarolineConcatto + rebase

Rebased on top of main

Tidy-up include paths

This was pointed out by @CarolineConcatto offline, thank you!

Thank you @awarzynski for implementing fsyntax-only.
It is another feature that the new driver provides, it is nice to see thing falling into place.
The patch looks good to me!

flang/include/flang/Frontend/FrontendActions.h
13

Do you need this library here?

This revision was landed with ongoing or failed builds.Dec 18 2020, 1:48 AM
This revision was automatically updated to reflect the committed changes.