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

Unit TestsFailed

TimeTest
280 msx64 debian > libarcher.races::task-dependency.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c
330 msx64 debian > libarcher.races::task-taskgroup-unrelated.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c
260 msx64 debian > libarcher.races::task-taskwait-nested.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c
280 msx64 debian > libarcher.races::task-two.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-two.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-two.c
390 msx64 debian > libarcher.task::task-barrier.c
Script: -- : 'RUN: at line 15'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/task/task-barrier.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/task/Output/task-barrier.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/task/Output/task-barrier.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/task/Output/task-barrier.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/task/task-barrier.c
View Full Test Results (15 Failed)

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
4074–4079

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.

4112–4114

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
166–171

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)

182

Please, could you comment hard-coded args:

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

See LLVM's coding style

314–318

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
4074–4078

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.Feb 4 2021, 4:24 AM
This revision was automatically updated to reflect the committed changes.

Before this patch, clang would happily ignore a -ffree-form flag. With this patch, none of -Wno-error=unknown-argument, -Wno-unknown-argument , -Qunused-arguments help to avoid clang from exiting with

error: unknown argument: '-ffree-form'

Why can't clang ignore these flags as any other unknown flags?

As a background: in the build system I'm dealing with, I cannot avoid that fortran flags are passed to the linking command. As C++ and fortran is involved, I prefer using clang++ as the linking command and explicitly link the fortran runtime library (at the moment gfortran, but in the future probably the flang runtime library)

Hi @protze.joachim,

Thank you for your feedback. I'm sorry that this is causing issues in your set-up.

Before this patch, clang would happily ignore a -ffree-form flag.

It's a bit more nuanced than this. Originally, -ffree-form (and other similar flags) were added to be forwarded to gfortran. This forwarding was effectively switched off ~6 months before this patch, please see this commit. As such, these flags stopped being used/needed by Clang. This patch makes Flang "re-claim" them.

With this patch, none of -Wno-error=unknown-argument, -Wno-unknown-argument , -Qunused-arguments help to avoid clang from exiting with

error: unknown argument: '-ffree-form'

Why can't clang ignore these flags as any other unknown flags?

IIUC, this is not something specific to the options refactored in this patch. For example:

# Test -foo
clang-cl -c -Wno-unknown-argument -foo  file.c
clang -c -Wno-unknown-argument -foo  file.c
clang-13: error: unknown argument: '-foo'
# Test -ffree-form
clang-cl -c -Wno-unknown-argument -ffree-form  file.c
clang -c -Wno-unknown-argument -ffree-form  file.c
clang-13: error: unknown argument: '-ffree-form'

Basically, -Wno-unknown-argument is only honored in clang-cl. Also, it applies to any option rather then just the options modified here.
I'm not particularly familiar with the semantics of the diagnostics options that you listed (-Wno-error=unknown-argument, -Wno-unknown-argument , -Qunused-arguments), but I get the impression that in general clang does not ignore options that it does not know about. I couldn't find any example that would demonstrate otherwise.

As a background: in the build system I'm dealing with, I cannot avoid that fortran flags are passed to the linking command. As C++ and fortran is involved, I prefer using clang++ as the linking command and explicitly link the fortran runtime library (at the moment gfortran, but in the future probably the flang runtime library)

It sounds like your build system relies on the gfortran support in Clang, but this support has been "bit rotting". I'm keen to help you resolve your problems, but finding a solution that will work for clang, flang and gfortran might require some effort. Would you be able to step-up as Clang's gfortran-mode maintainer? (no pressure, just brainstorming!)
Alternatively, do you need ToT clang for your build system?

Thank you,
-Andrzej

protze.joachim added a comment.EditedMar 18 2021, 9:18 AM

Hi Andrzej,

thanks for the detailed insights. I think I really misunderstood, what the -W flags can do here. I also was not up-to-date regarding the state of the flang implementation.

I just found, that compiling Spec OMP 2012 with clang-12 worked fine with my configuration, but compiling with current HEAD failed. Bisecting brought me to this patch.
What my build configuration basically does is to use gfortran to compile Fortran source to object files. Then I use clang/clang++ to link all the object files, basically:

gfortran -c foo.f
clang --gcc-toolchain=$(dirname $(dirname $(which gcc))) -lgfortran foo.o

In the build configuration, I can set FPORTABILITY flags for specific apps, but these flags are unfortunately passed to both the compile and the link step. Setting FLD to clang used to work, and now it broke.

One of my reasons for linking with clang is to make sure, that as much LLVM libraries as possible are linked. Especially, I want to pick up the LLVM version of the ThreadSanitizer runtime.

I tried to change my build configuration to use flang. After fixing some of the code (removing save keyword from variables, when having a global save), I could successfully compile one of the codes. Since flang under the hood uses gfortran for linking, this configuration picks up the GNU version of the ThreadSanitizer runtime. The GNU runtime is typically built as a dynamic library and comes with ~30-40% performance penalty. So, even when compiling with flang, I would prefer to link using clang.

I have no idea about the compiler internals, but would it be possible to mark the flang flags as known to the compiler tool-chain, but not used in case of clang? Finally, this would result in a -Wunused-command-line-argument warning.

Best
Joachim

Thank you for you reply Joachim,

I see 3 options here:

OPTION 1: make Clang accept these options
This would be as simple as removing the FlangOnlyOption from the corresponding TableGen definitions in Options.td. However, since we added help texts to their definitions, these options would be displayed together with other options when using clang --help. Given that these options are _not supported_ by clang, I think that we should avoid this upstream. However, this could be a quick and easy fix for you downstream. More on clang -help in this RFC.

OPTION 2: tweak the driver
From your description, it seems like it would make more sense to tweak the driver so that it can work in .e.g _linker_ mode. In such a mode, it would skip the compilation phase and ignore the compilation flags altogether. IMHO this would be the most sound approach here, but it may require a bit of work. I would start by asking on cfe-dev - it's likely that more projects use clang as a linker driver. Perhaps it's already possible?

OPTION 3: tweak -Wno-unknown-argument
I would expect -Wno-unknown-argument to work here as you initially suggested. I'm not familiar with its semantics or e.g. DiagnosticDriverKinds.td (there's a lot in there!). But sometimes you can tweak these things with a one-line change and then stuff auto-magically works. I can take a look at some point next week. It might be a low hanging fruit or a dead end.

What are you thoughts?

Best,
Andrzej

Hi @protze.joachim ,

Interestingly, GCC does not _error_ when using gfortran flags:

$ gcc -ffree-form file.c
cc1: warning: command line option ‘-ffree-form’ is valid for Fortran but not for C

I've implemented similar behavior for Clang in https://reviews.llvm.org/D99353. Please, could you take a look and see whether that solves your problem?

-Andrzej