Page MenuHomePhabricator

[flang][driver] Add forced form flags and -ffixed-line-length
ClosedPublic

Authored by FarisRehman on Jan 26 2021, 10:51 AM.

Details

Summary

Add support for the following layout options:

  • -ffree-form
  • -ffixed-form
  • -ffixed-line-length=n (alias -ffixed-line-length-n)

Additionally remove options -fno-free-form and -fno-fixed-form as they were initially added to forward to gfortran but gfortran does not support these flags.

This patch adds the flag FlangOnlyOption to the existing options -ffixed-form, -ffree-form and -ffree-line-length- in Options.td. As of commit 6a75496836ea14bcfd2f4b59d35a1cad4ac58cee, these flags are not currently forwarded to gfortran.

The default fixed line length in FrontendOptions is 72, based off the current default in Fortran::parser::Options. The line length cannot be set to a negative integer, or a positive integer less than 7 excluding 0, consistent with the behaviour of gfortran.

This patch does not add -ffree-line-length-n as Fortran::parser::Options does not have a variable for free form columns.
Whilst fixedFormColumns variable is used in f18 for -ffree-line-length-n, f18 only allows -ffree-line-length-none/-ffree-line-length-0 and not a user-specified value. fixedFormcolumns cannot be used in the new driver as it is ignored in the frontend when dealing with free form files.

Summary of changes:

  • Remove -fno-fixed-form and -fno-free-form from Options.td
  • Make -ffixed-form, -ffree-form and -ffree-line-length-n FlangOnlyOption in Options.td
  • Create AddFortranDialectOptions method in Flang.cpp
  • Create FortranForm enum in FrontendOptions.h
  • Add fortranForm_ and fixedFormColumns_ to Fortran::frontend::FrontendOptions
  • Update fixed-form-test.f so that it guarantees that it fails when forced as a free form file to better facilitate testing.

Diff Detail

Event Timeline

FarisRehman created this revision.Jan 26 2021, 10:51 AM
FarisRehman requested review of this revision.Jan 26 2021, 10:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2021, 10:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
FarisRehman edited the summary of this revision. (Show Details)Jan 26 2021, 10:54 AM
FarisRehman added a project: Restricted Project.

Thank you for working on this @FarisRehman ! I've left a few inline comments, but nothing major. Really nice to see this being added and working as expected!

clang/include/clang/Driver/Options.td
4131–4132

This is a fairly long help message. This is what gfortran prints:

gfortran --help=joined | grep 'ffixed-line-length'
  -ffixed-line-length-<n>     Use n as character line width in fixed mode

A long version could be added using DocBrief field.

4164–4165

We can't have help text for these, can we? Perhaps worth mentioning in the commit message that this needs fixing. We are likely to experience this with other options too.

clang/lib/Driver/ToolChains/Flang.cpp
30–31

IIUC, you are adding Fortran dialect rather then preprocessor options here. See also gfortran docs: https://gcc.gnu.org/onlinedocs/gfortran/Fortran-Dialect-Options.html.

flang/include/flang/Frontend/FrontendOptions.h
77–87

[nit] Perhaps Source file layout above enum class FortranForm?

flang/lib/Frontend/CompilerInstance.cpp
151–159

Hm, unwanted TABs?

Also, please keep note of: https://reviews.llvm.org/D95464 (there should be no conflicts from what I can tell).

flang/lib/Frontend/CompilerInvocation.cpp
170–175

Could be simplified using the ternary operator:

opts.fortranForm_ = (currentArg->getOption().matches(clang::driver::options::OPT_ffixed_form) ? FortranForm::FixedForm : FortranForm::FreeForm;

To me this would be more concise, but semantically it's identical. So please decide!

(similar comment for the bit below)

186

Please, could you comment hard-coded args:

} else if (argValue.getAsInteger(/*Radix=*/10, columns)) {

See LLVM's coding style

318–322

Hm, wouldn't this work:

fortranOptions.isFixedForm = (frontendOptions.fortranForm_ == FortranForm::FixedForm) ? true : false;
flang/test/Flang-Driver/Inputs/fixed-line-length-test.f
4

FIXME

flang/test/Flang-Driver/fixed-free-flag.f90
9–10

Current RUN lines are quite complicated and there's a lot going on. It might be safer to simplify them. Perhaps this:

! RUN: not %flang-new -fsyntax-only -ffree-form %S/Inputs/fixed-form-test.f  2>&1 | FileCheck %s --check-prefix=ERROR
! RUN: not %flang-new -fsyntax-only -ffixed-form %S/Inputs/free-form-test.f90  2>&1 | FileCheck %s --check-prefix=ERROR

So:

  • -ffree-form + fixed form file (extension and contents) --> Failure
  • -fixed-form + free form file (extension and contents) --> Failure

This is way you indeed test that the driver is forced to assume particular layout?

flang/test/Flang-Driver/fixed-line-length.f90
28

Could this be a regex instead?

33

The value is know, right? I would added it here. Also, the actual diagnostics printed is longer, isn't it?

Would it be possible to test negative numbers?

39

Regex?

FarisRehman marked 12 inline comments as done.

Address review comment

Address the review comment by @awarzynski

Summary of changes:

  • Shorten help text
  • Add method AddFortranDialectOptions to Flang.cpp
  • Change style of code to use quaternary operator
  • Update regression tests
FarisRehman edited the summary of this revision. (Show Details)Jan 28 2021, 5:48 AM
FarisRehman marked an inline comment as done.
FarisRehman added inline comments.
flang/lib/Frontend/CompilerInstance.cpp
151–159

I believe this is just Phabricator showing the comments have been indented

@FarisRehman, thank you for addressing my comments! I've just realised that gfortran doesn't actually support -fno-fixed-form or -fno-free-form:

$ gfortran -ffixed-form -fno-fixed-form test.f
gfortran: error: unrecognized command line option ‘-fno-fixed-form’

I've checked the other spellings too. If that's the case then perhaps the definitions in Options.td should be updated to reflect that? And if we don't use BooleanFFlag for these options then it should be much easier to add a help text too.

Remove -fno-fixed-form and -fno-free-form

Remove options -fno-fixed-form and -fno-free-form allowing for help messages to be added to -ffixed-form and -ffree-form.

FarisRehman edited the summary of this revision. (Show Details)Jan 29 2021, 8:29 AM
tskeith added inline comments.Jan 29 2021, 9:05 AM
clang/include/clang/Driver/Options.td
4131–4132

Why isn't this -ffixed-line-length=<value> (i.e. with an equals sign, not a hyphen)? Isn't that how clang does options with values? It's fine to support the gfortran form as an alternative but I think the primary goal should be good consistent style for options.

FarisRehman marked an inline comment as done.

Address review comment

This revision addresses a review comment by @tskeith

Summary of changes:

  • Add a new option, ffixed_line_length_EQ, that defines -ffixed-line-length=
  • Make option ffixed_line_length_VALUE (-ffixed-line-length-) an alias of ffixed_line_length_EQ, to be consistent with gfortran
  • Add flag FlangOnlyOption to ffixed-form, ffree-form and ffixed-line-length
FarisRehman edited the summary of this revision. (Show Details)Feb 2 2021, 9:08 AM
awarzynski accepted this revision.Feb 3 2021, 2:21 AM

Thank you for working on this @FarisRehman !

This revision is now accepted and ready to land.Feb 3 2021, 2:21 AM
This revision was landed with ongoing or failed builds.Thu, Feb 4, 4:24 AM
This revision was automatically updated to reflect the committed changes.