[flang] Change the default F18_FC to gfortran

[AlexisPerry]

Summary

Changed default F18_FC from pgf90 to gfortran. Removed unnecessary references to pgf90 in favor of more generic naming.

[AlexisPerry]

Extended the flang driver options to include gfortran equivalents to pgf90 specific options.

[clementval]

I don't see the previous change made to set gfortran as default. Are they gone with your diff update?

[AlexisPerry]

Changed the default external compiler used by the flang temporary driver.

• Changed default F18_FC from pgf90 to gfortran.
• Removed unneccesary references to pgf90 in favor of more generic naming.
• Extended the flang driver options to include gfortran equivalents to pgf90 specific options.

[AlexisPerry]

In D83488#2142950, @clementval wrote:

I don't see the previous change made to set gfortran as default. Are they gone with your diff update?

My mistake, sorry about that. They should be back now.

[clementval]

It's weird, the changes are in but both files are identical ... so it looks like there is nothing to review

[AlexisPerry]

I'll admit, this is my first time trying to commit to LLVM directly and using Phabricator (rather than on a fork that uses pull requests), and I'm definitely still learning the workflow. I had some git issues when first creating the patch that meant my first commit was lost locally, so when I went to update the patch it only had the second commit which caused the original changes to disappear. I then created a new branch and re-did the changes and updated again, using only a single commit, so that the diff would now have everything in it.

If this has resulted in an unreviewable state for this diff, then I will close it and submit a new one which will hopefully go more smoothly now that I've learned a few things. Sorry for the confusion.

[clementval]

In D83488#2142974, @AlexisPerry wrote:

I'll admit, this is my first time trying to commit to LLVM directly and using Phabricator (rather than on a fork that uses pull requests), and I'm definitely still learning the workflow. I had some git issues when first creating the patch that meant my first commit was lost locally, so when I went to update the patch it only had the second commit which caused the original changes to disappear. I then created a new branch and re-did the changes and updated again, using only a single commit, so that the diff would now have everything in it.

If this has resulted in an unreviewable state for this diff, then I will close it and submit a new one which will hopefully go more smoothly now that I've learned a few things. Sorry for the confusion.

The diff looks fine now! If you don't use it already, I would recommend to use arcanist to deal with the diff and phabricator ... it makes things easier

[clementval]

LGTM. You might want to wait to see if anybody has something to say on this.

[richard.barton.arm]

Thanks for picking up the work on this one @AlexisPerry!

I have replied to the RFC (late - my apologies) with a slightly different proposal. I'd like to make a final decision at the developers call on Monday. I don't hold my position too strongly though and would be happy to go in this direction. I also think we can go ahead with this patch because it moves us to a better place than we are now. Even if we chose my proposal to have no default most of this patch would still be kept because having gfortran support is a useful feature anyway.

On the patch itself, I have two requests:

1. This patch seems to be doing a couple of different things now that don't seem to be strictly linked:
   • Switching the default F18_FC to gfortran
   • Add support for a number of new gfortran options

I think these would be better submitted as two patches. Do you agree?

2. The patch should add a regression test for these fallback and option translation behaviours in test/Driver. It is a shame there are none already.

[clementval]

+1 to split this in two patches

[AlexisPerry]

Splitting this into two patches as requested by reviewers.

Change the default external compiler used by the flang temporary driver

- Changed default F18_FC from pgf90 to gfortran.
- Removed unneccesary references to pgf90 in favor of more generic naming.

[AlexisPerry]

The second part of the split can be found here: https://reviews.llvm.org/D83687

[richard.barton.arm]

Request one change to OpenMP handling, but the rest looks fine.

flang/tools/f18/f18.cpp

636

This needs to be amended to support gfortran. We'd need a similar switch on isPGF90 to pass -fopenmp here. I think it might work better to be moved into the next if/else block so it can be set to gfortran/pgfortran there.

Might also be better to add this as part of D83687 rather than this patch.

[AlexisPerry]

@richard.barton.arm I have made the requested adjustments in the other patch. Thanks for the feedback!

flang/tools/f18/f18.cpp

636

I agree this would be better as part of D83687, so I will add the fix over there. Good catch!

[tskeith]

When I build with this change I'm seeing a couple of modfile tests failing. The problem is that when isPGF90 is false, parser::useHexadecimalEscapeSequences is set to be true which affects how escape sequences are written in character literals in modfiles.

The choice of the compiler shouldn't be affecting modfile format, so I've created https://reviews.llvm.org/D83703 to fix that.

[AlexisPerry]

Updates following a rebase on master (due to changes on D83687)

[AlexisPerry]

@richard.barton.arm Is this patch acceptable now? Thanks!

[richard.barton.arm]

LGTM - thanks for doing this one for splitting the patch!

flang/tools/f18/f18.cpp

 //===-- tools/f18/f18.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
struct DriverOptions {
   bool dumpUnparseWithSymbols{false};
   bool dumpParseTree{false};
   bool dumpPreFirTree{false};
   bool dumpSymbols{false};
   bool debugResolveNames{false};
   bool debugNoSemantics{false};
   bool debugModuleWriter{false};
   bool measureTree{false};
-  bool unparseTypedExprsToPGF90{false};
-  std::vector<std::string> pgf90Args;
+  bool unparseTypedExprsToF18_FC{false};
+  std::vector<std::string> F18_FCArgs;
   const char *prefix{nullptr};
   bool getDefinition{false};
   GetDefinitionArgs getDefinitionArgs{0, 0, 0};
   bool getSymbolsSources{false};
 };
 
 bool ParentProcess() {
   if (fork() == 0) {
void Exec(std::vector<char *> &argv, bool verbose = false) {
   execvp(argv[0], &argv[0]);
   llvm::errs() << "execvp(" << argv[0]
                << ") failed: " << llvm::sys::StrError(errno) << '\n';
   exit(EXIT_FAILURE);
 }
 
 void RunOtherCompiler(DriverOptions &driver, char *source, char *relo) {
   std::vector<char *> argv;
-  for (size_t j{0}; j < driver.pgf90Args.size(); ++j) {
-    argv.push_back(driver.pgf90Args[j].data());
+  for (size_t j{0}; j < driver.F18_FCArgs.size(); ++j) {
+    argv.push_back(driver.F18_FCArgs[j].data());
   }
   char dashC[3] = "-c", dashO[3] = "-o";
   argv.push_back(dashC);
   argv.push_back(dashO);
   argv.push_back(relo);
   argv.push_back(source);
   Exec(argv, driver.verbose);
 }
std::snprintf(tmpSourcePath, sizeof tmpSourcePath, "/tmp/f18-%lx.f90",
     if (EC) {
       llvm::errs() << EC.message() << "\n";
       std::exit(EXIT_FAILURE);
     }
     Unparse(tmpSource, parseTree, driver.encoding, true /*capitalize*/,
         options.features.IsEnabled(
             Fortran::common::LanguageFeature::BackslashEscapes),
         nullptr /* action before each statement */,
-        driver.unparseTypedExprsToPGF90 ? &asFortran : nullptr);
+        driver.unparseTypedExprsToF18_FC ? &asFortran : nullptr);
   }
 
   if (ParentProcess()) {
     filesToDelete.push_back(tmpSourcePath);
     if (!driver.compileOnly && driver.outputPath.empty()) {
       filesToDelete.push_back(relo);
     }
     return relo;
std::string CompileOtherLanguage(std::string path, DriverOptions &driver) {
   }
   RunOtherCompiler(driver, path.data(), relo.data());
   return {};
 }
 
 void Link(std::vector<std::string> &relocatables, DriverOptions &driver) {
   if (!ParentProcess()) {
     std::vector<char *> argv;
-    for (size_t j{0}; j < driver.pgf90Args.size(); ++j) {
-      argv.push_back(driver.pgf90Args[j].data());
+    for (size_t j{0}; j < driver.F18_FCArgs.size(); ++j) {
+      argv.push_back(driver.F18_FCArgs[j].data());
     }
     for (auto &relo : relocatables) {
       argv.push_back(relo.data());
     }
     if (!driver.outputPath.empty()) {
       char dashO[3] = "-o";
       argv.push_back(dashO);
       argv.push_back(driver.outputPath.data());
     }
     Exec(argv, driver.verbose);
   }
 }
 
 int main(int argc, char *const argv[]) {
 
   atexit(CleanUpAtExit);
 
   DriverOptions driver;
-  const char *pgf90{getenv("F18_FC")};
-  driver.pgf90Args.push_back(pgf90 ? pgf90 : "pgf90");
-  bool isPGF90{driver.pgf90Args.back().rfind("pgf90") != std::string::npos};
+  const char *F18_FC{getenv("F18_FC")};
+  driver.F18_FCArgs.push_back(F18_FC ? F18_FC : "gfortran");
+  bool isPGF90{driver.F18_FCArgs.back().rfind("pgf90") != std::string::npos};
 
   std::list<std::string> args{argList(argc, argv)};
   std::string prefix{args.front()};
   args.pop_front();
   prefix += ": ";
   driver.prefix = prefix.data();
 
   Fortran::parser::Options options;
#endif
   while (!args.empty()) {
     std::string arg{std::move(args.front())};
     args.pop_front();
     if (arg.empty()) {
     } else if (arg.at(0) != '-') {
       anyFiles = true;
       auto dot{arg.rfind(".")};
       if (dot == std::string::npos) {
-        driver.pgf90Args.push_back(arg);
+        driver.F18_FCArgs.push_back(arg);
       } else {
         std::string suffix{arg.substr(dot + 1)};
         if (suffix == "f" || suffix == "F" || suffix == "ff" ||
             suffix == "f90" || suffix == "F90" || suffix == "ff90" ||
             suffix == "f95" || suffix == "F95" || suffix == "ff95" ||
             suffix == "cuf" || suffix == "CUF" || suffix == "f18" ||
             suffix == "F18" || suffix == "ff18") {
           fortranSources.push_back(arg);
if (arg.empty()) {
       options.instrumentedParse = true;
     } else if (arg == "-fdebug-semantics") {
     } else if (arg == "-fdebug-no-semantics") {
       driver.debugNoSemantics = true;
     } else if (arg == "-funparse") {
       driver.dumpUnparse = true;
     } else if (arg == "-funparse-with-symbols") {
       driver.dumpUnparseWithSymbols = true;
-    } else if (arg == "-funparse-typed-exprs-to-pgf90") {
-      driver.unparseTypedExprsToPGF90 = true;
+    } else if (arg == "-funparse-typed-exprs-to-f18-fc") {
+      driver.unparseTypedExprsToF18_FC = true;
     } else if (arg == "-fparse-only") {
       driver.parseOnly = true;
     } else if (arg == "-c") {
       driver.compileOnly = true;
     } else if (arg == "-o") {
       driver.outputPath = args.front();
       args.pop_front();
     } else if (arg.substr(0, 2) == "-D") {
if (arg.empty()) {
           << "  -v -c -o -I -D -U    have their usual meanings\n"
           << "  -help                print this again\n"
           << "Other options are passed through to the compiler.\n";
       return exitStatus;
     } else if (arg == "-V") {
       llvm::errs() << "\nf18 compiler (under development)\n";
       return exitStatus;
     } else {
-      driver.pgf90Args.push_back(arg);
+      driver.F18_FCArgs.push_back(arg);
       if (arg == "-v") {
         driver.verbose = true;
       } else if (arg == "-I") {
-        driver.pgf90Args.push_back(args.front());
+        driver.F18_FCArgs.push_back(args.front());
         driver.searchDirectories.push_back(args.front());
         args.pop_front();
       } else if (arg.substr(0, 2) == "-I") {
         driver.searchDirectories.push_back(arg.substr(2));
       }
     }
   }
 
   if (driver.warnOnNonstandardUsage) {
     options.features.WarnOnAllNonstandard();
   }
   if (options.features.IsEnabled(Fortran::common::LanguageFeature::OpenMP)) {
-    driver.pgf90Args.push_back("-mp");
+    driver.F18_FCArgs.push_back("-mp");
   }
   if (isPGF90) {
     if (!options.features.IsEnabled(
             Fortran::common::LanguageFeature::BackslashEscapes)) {
-      driver.pgf90Args.push_back(
+      driver.F18_FCArgs.push_back(
           "-Mbackslash"); // yes, this *disables* them in pgf90
     }
     Fortran::parser::useHexadecimalEscapeSequences = false;
   } else {
     if (options.features.IsEnabled(
             Fortran::common::LanguageFeature::BackslashEscapes)) {
-      driver.pgf90Args.push_back("-fbackslash");
+      driver.F18_FCArgs.push_back("-fbackslash");
     }
     Fortran::parser::useHexadecimalEscapeSequences = true;
   }
 
   if (!anyFiles) {
     driver.measureTree = true;
     driver.dumpUnparse = true;
     CompileFortran("-", options, driver, defaultKinds);