Page MenuHomePhabricator

[Flang][Driver] Add PrintPreprocessed FrontendAction
ClosedPublic

Authored by awarzynski on Sep 27 2020, 10:33 AM.

Details

Summary

[Flang][Driver] Add PrintPreprocessed FrontendAction

This patch implements the first frontend action for the Flang parser
(i.e. Fortran::parser). This action runs the preprocessor on the input
Fortran file and prints the generated output either to stdout or the
output file (specified with - or -o <output-file>).

Note that currently there is no mechanism to map options for the
frontend driver (i.e. Fortran::frontend::FrontendOptions) to options for
the parser (i.e. Fortran::parser::Options). Instead,
Frotran::parser::options are hard-coded to:

std::vector<std::string> searchDirectories{"."s};
searchDirectories = searchDirectories;
isFixedForm = false;
_encoding(Fortran::parser::Encoding::UTF_8);

These default settings are compatible with the current driver. Further
work is required in order for CompilerInvocation to read and map
clang::driver::options to Fortran::parser::options.

Co-authored-by: Andrzej Warzynski <andrzej.warzynski@arm.com>

Depends on D87989

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
CarolineConcatto requested review of this revision.Sep 27 2020, 10:33 AM

Remove -E from FlangOptions

awarzynski added a project: Restricted Project.

Rebase + refector the unit test

This is re-based on top of the latest version of D87989. The unit test is
update for consistency with D87989.

awarzynski commandeered this revision.Oct 16 2020, 7:53 AM
awarzynski edited reviewers, added: CarolineConcatto; removed: awarzynski.

Since @CarolineConcatto has recently moved to a different project, I am assigning this to myself and will be responding to the future review comments. Thank you for all the effort @CarolineConcatto !

awarzynski retitled this revision from [Flang][Driver]Add PrintPreprocessedInput action `-E` to [Flang][Driver] Add PrintPreprocessed FrontendAction.Oct 16 2020, 7:57 AM
awarzynski edited the summary of this revision. (Show Details)

Thank you, this patch looks easy to understand as it's clearly separated from(D87989) the infrastructure changes needed for frontend actions.
A few comments inline.

flang/include/flang/Frontend/CompilerInstance.h
85

If I am correct this seems to be an accessor member function and it should follow point 3 from flang style guide mentioned at https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#naming

I am not aware if driver related patches follow llvm-project style.

flang/include/flang/Frontend/CompilerInvocation.h
12

Nit: I believe clang-format is missing.

flang/lib/Frontend/CMakeLists.txt
17 ↗(On Diff #298635)

I believe this patch is a parsing and preprocessing related patch, is this used somewhere or should this be removed for now?

flang/lib/Frontend/CompilerInstance.cpp
136

May be you meant as it is used?

flang/lib/Frontend/FrontendAction.cpp
8

undo

58

Nit: Converts to phases in english according to my browser. :)

flang/lib/Frontend/FrontendActions.cpp
62

https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#c-language
point 4
Can you use braced initializers ?

A more better version I think would be to simplify this to

if (auto os{ci.CreateDefaultOutputFile(/*Binary=*/true, /*InFile=*/GetCurrentFileOrBufferName())}){
    (*os) << out.str();
} else {
  llvm::errs() << "Unable to create the output file\n";
  return;
}
flang/test/Frontend/Inputs/hello-world.c
2

a - ?

awarzynski marked 3 inline comments as done.

Address PR comments, make clearer separation between option types, simplify/fix tests

Based on the feedback, I've made a clearer separation between the parser and
the frontend driver options. I've also emphasised that mapping between the two
is a TODO. Initialisation to the defaults has been extracted out of
ExecuteAction (these should be set before an action is run).

I've also fixed a bug in the unit test and simplified the regression tests.
There's no need to differentiation between:

  • -E -, -E -o <output-file> and -E

That is done in https://reviews.llvm.org/D87989. In this patch it is sufficient
to focus on testing -E.

Thank you for reviewing @sameeranjoshi !

awarzynski added inline comments.Oct 19 2020, 10:24 AM
flang/include/flang/Frontend/CompilerInstance.h
85

Thanks, good catch!

Flang driver is a subproject within Flang and we've assumed that it should follow Flang's coding style. I'm much more used to LLVM's coding style and also spend a lot of time in Clang - hence mistakes like this.

Personally I think that it's unfortunate that there are multiple coding styles within llvm-project. But this is neither time nor place to challenge that :)

Btw, sadly we've made similar mistake in other places: https://github.com/llvm/llvm-project/blob/master/flang/include/flang/Frontend/CompilerInstance.h#L30. I will refactor that in a separate patch.

flang/include/flang/Frontend/CompilerInvocation.h
12

I did apply it and this line looks OK to me - what's missing?

49–52

This comment should clearly communicate that these are _frontend_ options. frontendOpts_ are _frontend_ driver options. The naming is perhaps a bit unfortunate :/

flang/lib/Frontend/CMakeLists.txt
17 ↗(On Diff #298635)

Not sure how that crept in - thank you!

flang/lib/Frontend/CompilerInstance.cpp
136

Good point :)

I have even more drastic suggestion - IMO the defaults should be set in a dedicated method. And the encoding for files could be set in the constructor of CompilerInstance.

I will implement that change. I will also add comments to highlight that we are using these defaults during the development. However, the end goal is to provide APIs for the user the set-up the frontend options.

flang/lib/Frontend/FrontendAction.cpp
58

Fixed :)

flang/lib/Frontend/FrontendActions.cpp
62

Neat suggestion, though: https://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor. For consistency sake, I will follow the convention from Flang. I will also simplify this function a bit.

flang/test/Frontend/print-preprocess-C-file.f90
9

Probably copied from a different file - can be removed.

11–12

These 2 tests are no different from the previous one. The key thing to test here is that flang-new -E triggers an error for C files. I recommend removing that.

flang/test/Frontend/print-preprocessed-file.f90
10–11

Similar comment as for print-preprocess-C-file.f90 (i.e. only one version is needed).

flang/unittests/Frontend/PrintPreprocessedTest.cpp
10

Not sure how this crept in here. Need to delete.

73

This should be startswith instead of equals. The output buffer is not initialized and hence there will be garbage past "program b".`

Apply fix to unit test that got lost in the previous patch, simplify C input file

A few nits: and mostly style comments inline:

flang/include/flang/Frontend/CompilerInvocation.h
12

See point 2[1] below the code block.
Ideally clang comes alphabetically first.

[1] https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#files

flang/include/flang/Frontend/FrontendActions.h
9

Why is this not following the style guide mentioned at [1] point 2.

[1] https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#files

awarzynski added inline comments.Oct 27 2020, 9:59 AM
flang/include/flang/Frontend/CompilerInvocation.h
12

Thanks for pointing this out! I appreciate that Flang has its own coding style, but I think that in terms of the order of include files, the rules from LLVM's coding standard [1] and Flang's are actually similar. From [1]:

We prefer these #includes to be listed in this order:

Main Module Header
Local/Private Headers
LLVM project/subproject headers (clang/..., lldb/..., llvm/..., etc)
System #includes
and each category should be sorted lexicographically by the full path.

This is then further clarified as:

LLVM project and subproject headers should be grouped from most specific to least specific, for the same reasons described above. For example, LLDB depends on both clang and LLVM, and clang depends on LLVM. So an LLDB source file should include lldb headers first, followed by clang headers, followed by llvm headers, to reduce the possibility (for example) of an LLDB header accidentally picking up a missing include due to the previous inclusion of that header in the main source file or some earlier header file. clang should similarly include its own headers before including llvm headers. This rule applies to all LLVM subprojects.

That's the rule that I applied here. This is consistent with the current .clang-format configuration for Flang: https://github.com/llvm/llvm-project/blob/d26dd743084a886382204ede5eeed146cd29fcd6/flang/.clang-format#L10-L18. This is also why clang-format believes this is correct.

In other words, there are two rules:

  • project specific header files first (i.e. header files from Flang first)
  • then, use alphabetic order.

I do feel that both [1] and [2] leave room for interpretation. If we are still in disagreement, we can follow-up on flang-dev or in one of the weekly calls. I'm keen to get this right!

[1] https://llvm.org/docs/CodingStandards.html#include-style
[2] https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md

flang/include/flang/Frontend/FrontendActions.h
9

I'm guessing that you are referring to this rule specifically:

Header files should be idempotent. Use the usual technique:

#ifndef FORTRAN_header_H_
#define FORTRAN_header_H_
// code
#endif  // FORTRAN_header_H_

To me this rule reads: "use hash guards". The rule doesn't mention what the format of the hash guard should be. However, LLVM's style guide does: https://llvm.org/docs/CodingStandards.html#header-guard. We've adopted it based on the following rule from Flang's style guide:

Otherwise, where LLVM's C++ style guide is clear on usage, follow it.

The format of the hash guards hasn't been contested in any of the previous reviews, so we assume that the format is fine.

And if this is not about the format of the hash guard - please clarify :)

Looks good, I would wait for a couple of more days for someone to review from community may be from Nvidia's side if someone would verify the initial design.
Thank you.

flang/include/flang/Frontend/CompilerInvocation.h
12

Thanks for details.
I was unaware of the fact that local project headers should come first, I was assuming everything is lexicographically sorted.

flang/include/flang/Frontend/FrontendActions.h
9

I reverted back seeing the clang-tidy warnings and no FORTRAN prefix but a LLVM_FLANG prefix.
If it was approved in previous reviews, I think that OK to keep it.

sameeranjoshi accepted this revision.Oct 27 2020, 11:09 AM
This revision is now accepted and ready to land.Oct 27 2020, 11:09 AM

Rebase on top of master + rename accessor method

This revision was landed with ongoing or failed builds.Mon, Nov 2, 6:03 AM
This revision was automatically updated to reflect the committed changes.

I would wait for a couple of more days for someone to review from community may be from Nvidia's side if someone would verify the initial design.

As there were no new reviews, I assumed that this good to land and have just merged it. I encourage people to submit post-commit reviews if they identify any issues with this patch that we've missed.

Thank you all for reviewing!