Page MenuHomePhabricator

[flang][driver] Add options for -std=f2018
ClosedPublic

Authored by arnamoy10 on Feb 20 2021, 7:52 AM.

Details

Summary

Add support for the following Fortran dialect options:

-std=f2018

It also adds one test cases. Currently only f2018 is allowed as the standard.

Diff Detail

Event Timeline

arnamoy10 created this revision.Feb 20 2021, 7:52 AM
arnamoy10 requested review of this revision.Feb 20 2021, 7:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2021, 7:52 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Please make sure the test works with f18 also.

flang/lib/Frontend/CompilerInvocation.cpp
397

This should be "f2018", not "2018".

flang/test/Flang-Driver/std2018.f90
18 ↗(On Diff #325205)

Can you verify this message is not produced when there is no -std option?

Hi @arnamoy10 , thank for working on this!

I suggest making it more explicit that the new option is about _enabling standard conformance checks_. So e.g. enableConformanceChecks rather than setStandard (or set_EnableConformanceChecks if the member variable is called EnableConformanceChecks).

I am also mindful that adding -std=f2018 might be a bit confusing to users if we don't support other versions of the language. @tskeith raised this point earlier.

Perhaps we should use e..g -fstd-conf-checks/-fno-std-conf-checks instead? This could be an alias for -Mstandard in f18. We would replace it with -std=<arg> once that makes more sense.

flang/include/flang/Frontend/CompilerInvocation.h
76

Wouldn`t e.g. standardConformanceChecks_ be a bit more self-explanatory? Currently this is quite vague.

124

Missing empty line.

flang/test/Flang-Driver/std2018.f90
12–13 ↗(On Diff #325205)

[nit] -fstynax-only is effectively the default in the frontend driver, see here. Basically, in the absence of any _action_ options, the driver will run the ParseSyntaxOnly action (which corresponds to -fsyntax-only)

  1. Updated the test case to include a check when the option is not given and make sure works with f18 as well (with Mstandard)
  2. Changed variable names as suggested.
arnamoy10 retitled this revision from [flang][driver] Add options for -std=2018 to [flang][driver] Add options for -std=f2018.Mar 1 2021, 1:55 PM
arnamoy10 edited the summary of this revision. (Show Details)

I agree with @tskeith that -Mstandard is not exactly orthogonal to -std, the former being about warning for non-standard extensions/deviations and the latter being about use of standard fortran, but from a different standard version. I would expect -Mstandard to apply to whatever value of -std you gave the compiler.

I think the gfortran analog to -Mstandard is -fpedantic or -fpedantic-errors - see https://gcc.gnu.org/onlinedocs/gfortran/Error-and-Warning-Options.html#Error-and-Warning-Options. The docs have a little joke in them about the feature not doing what users want, but I think that's just a witty way to say that the feature is incomplete, but that it is expected to catch things in theory. So maybe -fpedantic{-errors} is a good thing to substitute for -Mstandard?

FWIW, I think supporting -std with only one supported option is a reasonable thing to do and will help with folks porting codes to flang from gfortran to accept the option and to know what's supported and what is not. Perhaps the perfect thing would be supporting all the sensible gfortran values for -std, but emitting a (downgradable) warning or remark when one that is not supported is given. That would let users know what they are dealing with until the day when flang supports other values for -std. I doubt supporting older versions is on anyone's short-term roadmap, but patches would probably be welcomed on that. I guess there will one day be a -std=f202X option supported too!

I agree with @richard.barton.arm that we could add -std= now, even if only for a subset of possible language standards. I expect that standard conformance checks would be quite pervasive, and it would be better to design the infrastructure for such checks earlier rather than try to retrofit them at a later stage when we have to start supporting f202x, etc.

ChinouneMehdi added a project: Restricted Project.Mar 5 2021, 10:50 AM
ChinouneMehdi added a subscriber: flang-commits.

Updating the patch to make -pedantic and -f2018 behave the same way, also updated the test case accordingly.

awarzynski added inline comments.Mar 15 2021, 4:55 AM
clang/include/clang/Driver/Options.td
3538–3540

As this options is shared with Clang, the description should be generic enough so that it works for both clang and flang. Some sources for inspiration below:

gfortran 1

gfortran --help=common | grep "\-\-pedantic"
 --pedantic                  Same as -Wpedantic.  Use the latter option instead.
 --pedantic-errors       Same as -pedantic-errors.  Use the latter option instead.

gfortran 2

$ gfortran --help=common | grep "\-Wpedantic "
  -Wpedantic                  Issue warnings needed for strict compliance to the standard.

clang
(from https://clang.llvm.org/docs/UsersManual.html):

-pedantic
Warn on language extensions.

-pedantic-errors
Error on language extensions.
clang/lib/Driver/ToolChains/Flang.cpp
54

-pedantic controls errors and warning messages. It's not really a dialect option.

flang/lib/Frontend/CompilerInvocation.cpp
392–395

DELETEME?

flang/test/Driver/std2018.f90
7–12

We are moving towards sharing all of the tests between the new and the old drivers (more context: https://reviews.llvm.org/D98257). We should avoid:

  • %f18, which is meant for f18 only,
  • %flang-new, which is meant for flang-new only.

So this whole section is a bit problematic. As these options don't require any particular logic in the compiler driver (i.e. flang-new), I suggest deleting it. It is sufficient to test with the frontend driver %flang_fc1 (i.e. flang-new -fc1).

  1. Update the test case to only test flang-new -fc1
  2. Updated help text
  3. Clang-formatted and also removed redundant code
tskeith added inline comments.Mar 15 2021, 1:59 PM
flang/test/Driver/std2018.f90
10

You need to make sure these work with f18 or indicate the test requires the new driver. f18 doesn't currently support -std=f2018 and doesn't complain about unrecognized options.

arnamoy10 updated this revision to Diff 330823.Mar 15 2021, 3:20 PM

Adding one more update that is probably required in semanticContext (inspired by f18), also updated test case to require flang-driver

arnamoy10 updated this revision to Diff 330973.Mar 16 2021, 6:56 AM

Moving -std to non-dialect option

arnamoy10 updated this revision to Diff 331871.Mar 19 2021, 7:21 AM

Separating test cases for accepted input option and not accepted input option.

awarzynski added inline comments.Mar 19 2021, 9:47 AM
flang/test/Driver/std2018.f90
2

Would it be possible to share this test with f18?

flang/test/Driver/std2018_wrong.f90
15–23

No input is needed for this test, is it? IIUC, this bit can be deleted to make the test leaner.

arnamoy10 added inline comments.Mar 19 2021, 12:03 PM
flang/test/Driver/std2018.f90
2

Would you please elaborate on how do want the sharing to happen (same test file/different test file)? I think in a previous comment you mentioned that it is sufficient to test the frontend driver for this patch?

flang/test/Driver/std2018_wrong.f90
15–23

Sure, will do.

awarzynski added inline comments.Mar 19 2021, 12:20 PM
flang/test/Driver/std2018.f90
2

This line means that the test is not run when FLANG_BUILD_NEW_DRIVER is Off:

! REQUIRES: new-flang-driver

Once this line is removed, the test is effectively shared.

Currently this test won't work with f18(i.e. when you remove this line), but is should be sufficient to add -pedantic and -std=f2018 as aliases for Mstandard.

arnamoy10 updated this revision to Diff 332703.Tue, Mar 23, 9:39 AM
  1. Leaning out the "wrong-option" test case
  2. Adding aliases in f18 (-pedantic and -std=2018 for -Mstandard)
arnamoy10 updated this revision to Diff 332991.Wed, Mar 24, 7:51 AM

Rebasing on top of main

awarzynski accepted this revision.Wed, Mar 24, 8:22 AM

Thank you for updating this @arnamoy10!

One thing worth pointing out - this patch adds -pedantic rather than -fpendatic as @richard.barton.arm suggested. That was clearly a typo, so everything is good.

IMO this is ready to land provided that:

  • the test is updated to work with f18
  • summary/commit message is updated to reflect the recent changes (i.e. that this patch adds -std=f2018 AND -pedantic)

These are small changes and I'm happy for you to apply them when merging (rather than updating here first).

flang/lib/Frontend/CompilerInvocation.cpp
375

[nit] -pedantic instead

535

[nit] Comment inconsistent with the code (no standard is set here)

flang/test/Driver/std2018.f90
7–9

flang-new -fc1 (akin to clang- cc1), runs -fsyntax-only when no other action flag is specified. f18 will go straight into code-generation. As such, this test fails for f18. Adding -fsyntax-only should fix it. It will also make the test a bit more clearer.

This revision is now accepted and ready to land.Wed, Mar 24, 8:22 AM
arnamoy10 updated this revision to Diff 333311.Thu, Mar 25, 8:13 AM

Updating test case to add -fsyntax-only to share with f18

This revision was landed with ongoing or failed builds.Thu, Mar 25, 10:05 AM
This revision was automatically updated to reflect the committed changes.