Page MenuHomePhabricator

[flang][driver] Support fixed form detection
AcceptedPublic

Authored by FarisRehman on Thu, Jan 7, 4:47 AM.

Details

Summary

Currently the new flang driver always runs in free form mode. Add support for fixed form mode based on the file extensions that f18 looks for, and treat "for", "FOR", "fpp" and "FPP" file extensions as fixed form files like gfortran does.

Additionally add support for the missing file extensions "ff", "FOR", "for", "f77", "ff90", "fpp", "FPP" to be compatible with f18 in this respect, along with extensions "f03", "F03", "f08", "F08" as the Cray Fortran compiler supports it.

The following file extensions will be processed as fixed form: .f, .F, .ff, .for, .FOR, .fpp, .FPP
Any other file type will be processed as free form.

Summary of changes:

  • Add isFixedFormSuffix and isFreeFormSuffix to FrontendTool/Utils.h
  • Set Fortran::parser::Options#isFixedForm according to the file type
  • Change FrontendOptions#GetInputKindForExtension to support the missing file extensions that f18 supports and some additional file extensions

Diff Detail

Unit TestsFailed

TimeTest
160 msx64 windows > LLVM.tools/dsymutil/ARM::extern-alias.test
Script: -- : 'RUN: at line 38'; c:\ws\w1\llvm-project\premerge-checks\build\bin\dsymutil.exe -oso-prepend-path C:\ws\w1\llvm-project\premerge-checks\llvm\test\tools\dsymutil\ARM/../Inputs C:\ws\w1\llvm-project\premerge-checks\llvm\test\tools\dsymutil\ARM/../Inputs/private/tmp/private_extern/private_extern.out -o C:\ws\w1\llvm-project\premerge-checks\build\test\tools\dsymutil\ARM\Output\extern-alias.test.tmp.dSYM --verbose 2>&1 | c:\ws\w1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w1\llvm-project\premerge-checks\llvm\test\tools\dsymutil\ARM\extern-alias.test

Event Timeline

FarisRehman created this revision.Thu, Jan 7, 4:47 AM
FarisRehman requested review of this revision.Thu, Jan 7, 4:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jan 7, 4:47 AM
FarisRehman added a project: Restricted Project.
sameeranjoshi requested changes to this revision.Sun, Jan 10, 11:48 PM
sameeranjoshi added inline comments.
flang/lib/Frontend/FrontendOptions.cpp
14–15

What about .f08 extension or maybe f03?
It seems to be in one of the popular compilers[1]

[1]https://github.com/llvm/llvm-project/blob/master/flang/docs/OptionComparison.md#notes

15

Why is this repeated from above Cases statement?
Why are all options not inside a single Cases statement?
If it's an issue mentioned from the comments, would it be better to differentiate the above options in 2 different Cases based on free and fixed form and add a one liner comment on top about the form they belong to?

This revision now requires changes to proceed.Sun, Jan 10, 11:48 PM
FarisRehman marked 2 inline comments as done.Mon, Jan 11, 7:47 AM
FarisRehman added inline comments.
flang/lib/Frontend/FrontendOptions.cpp
15

Correct, it is because of the issue mentioned in the comments.
I agree and I have made the changes to separate the cases based on form, thanks.

FarisRehman marked an inline comment as done.

Support more extensions

Further support more extensions, and address the comments made by @sameeranjoshi

Summary of changes:

  • Add support for f08, F08, f03 and F03 extensions
  • Seperate Cases based on fixed and free form
  • Fix a unit test failing due to the fixed form change
FarisRehman edited the summary of this revision. (Show Details)Mon, Jan 11, 8:01 AM

Are you able to make a small table for us to look at the whole list of supported fixed and free form sources?

I believe the "-x" option allows different language types to be supported. Do we think -x will differentiate between free/fixed, or do we anticipate adding special options to instruct the compiler to handle fixed/free source?

More information about other compilers is available here:
https://github.com/llvm/llvm-project/blob/main/flang/docs/OptionComparison.md

@sscalpone @sameeranjoshi Let me just point out that currently Flang does not differentiate between different versions of Fortran. So file.f95 and file.f03 is internally simply treated as Language::Fortran.

I believe the "-x" option allows different language types to be supported. Do we think -x will differentiate between free/fixed, or do we anticipate adding special options to instruct the compiler to handle fixed/free source?

gfortran uses -ffixed-form & -ffree-form and that's what we proposed for Flang [1] [2]. The table below effectively lists all options supported by -x. For Fortran we differentiate between TY_Fortran (regular Fortran input, not preprocessed) and TY_PP_Fortran (regular Fortran input, preprocessed). In order to differentiate fixed and free form source file with -x we'd have to extend clang::driver::types::ID. IMO such change is not yet required.

More information about other compilers is available here:
https://github.com/llvm/llvm-project/blob/main/flang/docs/OptionComparison.md

This document was very helpful when compiling [1] and [2]. Kudos to @PeteSteinfeld for preparing it!

[1] https://docs.google.com/spreadsheets/d/1JRe39lP_KhtkYxFEIvwrCFlE5v1Ofa_krOHI-XXXWPY/edit#gid=0
[2] https://lists.llvm.org/pipermail/flang-dev/2020-November/000588.html

flang/lib/Frontend/FrontendOptions.cpp
14–15

Thanks for the comment. I just wanted to point out that currently Flang does not differentiate between different versions of Fortran. Indeed, only F2018 grammar is mentioned in the docs: https://github.com/llvm/llvm-project/blob/main/flang/docs/f2018-grammar.md

So, whether other file extension are added or not, internally we are using Language::Fortran. That's unlikely to change any time soon. Adding more file extensions here is just a _syntactic_ sugar at this stage.

FarisRehman edited the summary of this revision. (Show Details)Tue, Jan 12, 6:28 AM

@FarisRehman , thank you for working on this!

I think that it would be useful to add a test like this:
Input name: `input.f:

program ShouldFail
end

Run line:

flang-new -E input.f

This should fail as flang-new will identify the input it as fixed-form, yet the actual file is free form.

flang/test/Flang-Driver/Inputs/free-form-test.f90
2

I think that fixed-free-detection.f90 will be easier to read if you use different name here. Perhaps instead of A, you could use: FixedForm and FreeForm?

flang/test/Flang-Driver/fixed-free-detection.f90
2

This tests verifies the driver correctly switches between the free and fixed forms (based on the file extension). This is tested by exploiting the fact that the preprocessor (more generally, the frontend, but not the driver itself) treats white-spaces differently for free and fixed form input files.

Would you mind clarifying this?

tskeith added inline comments.Tue, Jan 12, 5:16 PM
flang/lib/Frontend/FrontendAction.cpp
62

This set of fixed form suffixes is duplicated in FrontendOptions.cpp. I think it would be better two have two predicates, say IsFixedFormSuffix and IsFreeFormSuffix, so that the former can be used in both places.

awarzynski added inline comments.Thu, Jan 14, 3:24 AM
flang/lib/Frontend/FrontendAction.cpp
62

I would prefer if we kept it as is.

In FrontendOptions.cpp (in particular in GetInputKindForExtension), we don't intend to and should not differentiate between the fixed and free forms. Nor should we leave any comments that would suggest otherwise. GetInputKindForExtension is about differentiating between e.g. Fortran, CUDA, LLVM IR inputs. For reference, see similar method in Clang:

If something like IsFixedFormSuffix and IsFreeFormSuffix is introduced and used in GetInputKindForExtension, then that will suggest that the differentiation between fixed and free form is significant in that method. That's not the case. Also, we would have to abandon the llvm::StringSwitch API there, which IMO fits there very well.

The only reason that there are two cases for Language::Fortran in GetInputKindForExtension is that LLVM's Cases allows 9 entries at most:

There's more than 9 file extensions that will lead to Language::Fortran.

flang/lib/Frontend/FrontendOptions.cpp
15

@sameeranjoshi @FarisRehman

Could I suggest that we actually remove // Free form files and // Fixed form files? These comments suggest that the split into fixed and free form matters here, but that is not the case here (see also my reply to @tskeith 's comment).

The key comment here is this:

// FIXME: Currently this API allows at most 9 items per case.

Link to the API: https://github.com/llvm/llvm-project/blob/cbea6737d5130724c7c8cf8ee4ccf1c3dd099450/llvm/include/llvm/ADT/StringSwitch.h#L132-L137. This is the only reason why we have two cases here instead of one. Perhaps we should just update that comment:

//  llvm::StringSwithc::Cases allows at most 9 strings per entry and hence we need multiple cases for Language::Fortran.

I think that it would be useful to add a test like this:
Input name: `input.f:

program ShouldFail
end

Run line:

flang-new -E input.f

This should fail as flang-new will identify the input it as fixed-form, yet the actual file is free form.

Thanks for the suggestion! However, currently the driver does not print non-fatal errors so this test is not possible as incorrect input form is non-fatal.

FarisRehman marked 2 inline comments as done.Thu, Jan 14, 4:24 AM
FarisRehman added inline comments.
flang/lib/Frontend/FrontendOptions.cpp
15

Based on how the discussion evolved, I also feel that it is best to not have these comments here, and I have removed them. cc @sameeranjoshi

Update regression test

Update the regression test to address comments.

Summary of changes:

  • Rename test program names for greater readability
  • Expand on the description of the regression test
  • Remove comments separating fixed form from free form in FrontendOptions.cpp
tskeith added inline comments.Thu, Jan 14, 7:12 AM
flang/lib/Frontend/FrontendAction.cpp
62

In FrontendOptions.cpp (in particular in GetInputKindForExtension), we don't intend to and should not differentiate between the fixed and free forms. Nor should we leave any comments that would suggest otherwise. GetInputKindForExtension is about differentiating between e.g. Fortran, CUDA, LLVM IR inputs. For reference, see similar method in Clang:

If something like IsFixedFormSuffix and IsFreeFormSuffix is introduced and used in GetInputKindForExtension, then that will suggest that the differentiation between fixed and free form is significant in that method.

If GetInputKindForExtension contained code like:
if (IsFixedFormSuffix(suffix) || IsFreeFormSuffix(suffix)) { return Language::Fortran; }
it would suggest to me that source form is not significant as they are both being treated the same way.

Also, we would have to abandon the llvm::StringSwitch API there, which IMO fits there very well.

It would fit better if the API weren't so clumsy. Wanting to use a favorite class is not a reason to duplicate code.

I'm fine with a different approach, but we shouldn't be listing the fixed-form suffixes twice in different places.

FarisRehman added inline comments.Fri, Jan 15, 8:42 AM
flang/lib/Frontend/FrontendAction.cpp
62

From this discussion, it seems in this case moving away from StringSwitch and having helper methods isFixedFormSuffix and isFreeFormSuffix is the best solution. I have made these changes.

Add helper methods to Utils

Add methods isFixedFormSuffix and isFreeFormSuffix to address the comment from @tskeith

Summary of changes:

  • Create isFixedFormSuffix and isFreeFormSuffix methods in FrontendTool/Utils.h
  • Use isFixedFormSuffix and isFreeFormSuffix methods instead of StringSwitch in FrontendOptions#GetInputKindForExtension
FarisRehman edited the summary of this revision. (Show Details)Fri, Jan 15, 8:43 AM
sameeranjoshi accepted this revision.Sun, Jan 17, 11:44 PM

Thank you for working on this.
It build and tests successfully.
Please wait for other reviewers to accept.

flang/include/flang/FrontendTool/Utils.h
17

Do you really need this?

flang/lib/Frontend/FrontendAction.cpp
13

Is this really needed?

This revision is now accepted and ready to land.Sun, Jan 17, 11:44 PM
FarisRehman marked an inline comment as done.Tue, Jan 19, 4:07 AM
FarisRehman added inline comments.
flang/include/flang/FrontendTool/Utils.h
17

This is needed as llvm::StringRef is used in isFixedFormSuffix and isFreeFormSuffix methods.