[Clang] [Driver]Add logic to search for flang frontend

[CarolineConcatto]

Summary

This patch adds more logic to Flang.cpp to search for the
flang frontend.

A new flag(ffc-fortran-name) was created in this patch to
implement the logic in Flang driver.

The patch searches for the appropriated flang frontend path according to:
-if the flag ffc-fortran-name is being set use the name given by
the flag to search for the flang frontend path

• esle the frontend name is hard-coded to "flang", use that to

search the frontend path.

To set the custom fortran frontend this patch created a new internal driver
flag named "ffc-fortran-name" in Options.td.
The flag sets flang frontend for a custom fortran compiler. The logic for
ffc-fortran-name is the same as the one used by GNU to set ccc-gcc-name.

Signed-off-by: Caroline Concatto <[e-mail]>

[DavidTruby]

If what I've suggested above doesn't work then the patch lgtm as is

clang/lib/Driver/Driver.cpp

132–133

I think you can default this to "flang" here instead of "", and then you don't need to check if it is empty later (see my later comment)

clang/lib/Driver/ToolChains/Flang.cpp

13–14

Nit: stray space here

71–78

I think here you can just do FortranName = D.getFFCGenericFortranName();
and then
getToolChain().GetProgramPath(FortranName.c_str())
as long as you have defaulted to "flang" earlier.

[richard.barton.arm]

A few comments running through that need addressing.

In addition - have you checked the behaviour of this option with -Bprefix? Looking at the code for Driver.GetProgramPath, it seems like that would affect the behaviour of this feature so clang --driver-mode=flang -B<prefix> -ffc-fortran-name=foo might be expected to make clang call <prefix>foo. There also seems to be some logic with -target that might affect this. There are tests for this feature in test/Driver/B-opt.c that could be added to or adapted into a new test.

clang/include/clang/Driver/Options.td

267

I'm not sure about this name. My memory is not long enough to know what ccc stands/stood for but git blame suggests it may have been a precursor name to clang. Although it might seem odd to add new options like this given the name perhaps doesn't mean anything anymore, I suggest copying this convention and make this option start with -ccc. I also think flang would be better than fortran here as it better describes what the option is doing, i.e. telling clang what flang is called.

So recommend -ccc-flang-name for the option and commensurate changes to the internal variables to match.

clang/test/Driver/flang/clang-driver-2-frontend01.f90

12 ↗

(On Diff #242290)

Given that the RUN line runs clang with -### I don't think it is necessary for the flang binary that -ffc-fortran-name points to actually be present. Can't we simplify the test for the basic case to run with -ffc-fortran-name=foo and check for foo?

If it is required that the -ffc-fortran-name argument point to an executable file that exists it would be better to create files in the Inputs directory and point there.

14 ↗

(On Diff #242290)

I don't understand why you are checking for this option. Surely the only relevant check is the "ALL-LABEL" check at the top which checks for the flang subprocess invocation? Can you explain why you need this check?

18 ↗

(On Diff #242290)

Similarly, I don't understand the need for this check on the output, although it would be correct.

clang/test/Driver/flang/driver-2-frontend01.f90

3 ↗

(On Diff #242290)

Why does the driver name matter here. Unless I have overlooked something I would expect the functionality to be the same whatever the driver is called.

[CarolineConcatto]

clang/include/clang/Driver/Options.td

267

I really don't mind changing the name. The way it is stands ffc for: flang fc1
As I thought that ccc was for clang cc1

clang/lib/Driver/Driver.cpp

132–133

I tried to follow the pattern set by the other tools, like GNU.

clang/test/Driver/flang/clang-driver-2-frontend01.f90

12 ↗

(On Diff #242290)

It is possible to do. The problem is that getProgramPath will not be "tested", because the file does not exist, so no path will return.
It will only return the name given by the driver. Is that a good test?
Shouldn't we check if the path is also sent.
I believe that without the file being created this check would have to change:
ALL-LABEL: "{{[^"]*}}clang-driver-2-frontend01.f90.tmp1"

14 ↗

(On Diff #242290)

Mainly for sanity check as the patch add a new flag. The logic added does not mess with the previous flags, but I thought it was good to do at least a minimum check.

If changing I would add more tests like the ones made in flang.f90 and flang.F90 as the one in these files do not use ffc-fortran-name.

clang/test/Driver/flang/driver-2-frontend01.f90

3 ↗

(On Diff #242290)

Good catch, it looks I've missed this text when re-writing the tests.

[richard.barton.arm]

clang/include/clang/Driver/Options.td

267

As I thought that ccc was for clang cc1

Hmm - that might be right right. It is certainly as good a guess as any.
I was also thinking maybe it was "Cross CC" or something like that given the use of the word "native" in the help text.

Perhaps we should just not use the naming convention as we don't really understand it. Maybe -flang-fc1 or -fortran-fe might be a better name?

Anyhow - given what you say, I'll withdraw my quibbling on the name. It is an internal name after all. Happy to go with whatever you chose, including the original.

clang/test/Driver/flang/clang-driver-2-frontend01.f90

14 ↗

(On Diff #242290)

I agree with you that the check is correct, -emit-obj will indeed be emitted so the check is a correct one for clang's current behaviour.

Where I am coming from is that there are already tests that check that when clang is called without -c (or -S, etc.) that it passes -emit-obj to the frontend. This test should be checking that -ffc-fortran-name causes the frontend that clang calls to change. It's not really concerned with the -emit-obj behaviour. But, because you have this CHECK line in the code, the test depends on it to pass. Say we changed the spelling of that option -emit-obj or we changed the behaviour of clang in some way that causes -emit-obj no longer to be passed to the FE in these circumstances. This test would still fail if not updated, even though it is supposed to be unrelated to the feature being changed.

[DavidTruby]

LGTM but wait for someone else to approve

clang/lib/Driver/Driver.cpp

132–133

If you're following a pattern that is used in other parts of the code then this is fine by me.

[richard.barton.arm]

I'd still like to see the nits addressed and comments on the tests addressed before approving.

[CarolineConcatto]

@richard.barton.arm A new patch will land today with the changes asked by you.

[CarolineConcatto]

• [Clang]Rename flag and update the tests

[richard.barton.arm]

Still not sure why all the tests are needed here. The first one looks fine, i.e. we test that --fortran-fe=<path to copy of driver binary we just created> calls to that copy. The second one appears to test the default behaviour with no option, but why does it do it via a different name for clang, why would that make a difference? The second uses a different name for both driver and frontend. Why is that test relevant to the change you have made? As far as I can see, the name of the driver binary has no bearing on its behaviour wrt. calling a frontend binary. What am I missing?

clang/include/clang/Driver/Options.td

268

This is not right. I think the option points the driver at the name of the Fortran frontend to call. I don't think that has to be native or otherwise. Suggest changing this string to "Name for Fortran frontend"

clang/test/Driver/flang/clang-driver-2-frontend01.f90

10 ↗

(On Diff #244635)

Does %t1 not work again on this line?

clang/test/Driver/flang/flang-driver-2-frontend01.f90

10 ↗

(On Diff #244635)

No they shouldn't ?

[richard.barton.arm]

clang/include/clang/Driver/Options.td

268

I prefer the new option name though - thanks.

[CarolineConcatto]

clang/test/Driver/flang/clang-driver-2-frontend01.f90

10 ↗

(On Diff #244635)

If I don't create the fake link getProgramPath will only return the name, not the entire path.
t1 here is the path for the frontend.
For instance:
clang --driver-mode=flang -fortran-fe %test
the frontend name is test, but when running it should print:
<path-to-test>/test -fc1
without the link it will only print:
test -fc1
Like I said before it is more a preference that actually a requisite.

[richard.barton.arm]

clang/test/Driver/flang/clang-driver-2-frontend01.f90

10 ↗

(On Diff #244635)

Understood - thanks.

[sscalpone]

type in the commit message: esle the frontend name is hard-coded to "flang", use that to

[sscalpone]

clang/test/Driver/flang/flang-driver-2-frontend01.f90

1 ↗

(On Diff #244635)

typo wich

clang/test/Driver/flang/flang-driver-2-frontend02.f90

1 ↗

(On Diff #244635)

Typo

[awarzynski]

The suggested flag, -fortran-fe, feels quite powerful and I wonder whether it's required this early into the development of the Flang driver?

IIUC, this flag would be used to workaround the fact that currently flang is a bash script and hence can't be used when the driver is in the flang mode. -fortran-fe could be use to redirect the call to flang-tmp (the temporary flang driver that will eventually replace the bash script with similar name). Wouldn't it be less work if we just used flang-tmp instead of flang here: https://github.com/llvm/llvm-project/blob/42a56bf63f699a620a57c34474510d9937ebf715/clang/lib/Driver/ToolChains/Flang.cpp#L72? Once the driver is ready to replace the bash script, we could revert the change in Flang.cpp (flang-tmp -> flang).

Are there any other usecases that we should cater for at this point? Thanks!

[CarolineConcatto]

@awarzynski If we don't foresee a use for this flag beside the problem I've faced, then I am fine to remove and only replace by flang-tmp.

But as we discuss this flag enables flang driver to call another driver, basically cross-compilation.
That could be a useful flag in the future if we intend to do cross-compilation with Fortran.

My conclusion is that:

we should add  the flag if we agree that it will be needed in the future, even if it is not for now,
otherwise, I am happy to take the temporary path of replacing flang by flang-tmp.

[richard.barton.arm]

So, coming back to this after a few weeks break.

I agree with the point @awarzynski makes on this option being powerful and I think your alternative suggestion of using a different hard-coded name could work well at this stage of the project. I think the usecase that Carol and I were considering was for when building a clang/flang toolchain and renaming the binaries to something else. In that case, you would not want your driver to call to a hard-coded flang binary called "flang" but support calling to a flang binary called something else like "fooflang". In our case, we derive a commercial product from clang called armclang and I'm sure we are not the only ones doing something similar.

An additional thought I had was that we are in this situation today only because we need to name the binary "flang-tmp" because, as @awarzynski points out, "flang" is taken by the wrapper. This is because we are adding the new driver in the same place as the old driver, i.e. in flang/tools/f18. Firstly, that's a bad name for the new driver - f18 vs flang - and secondly perhaps it would be more clean to add the new driver as a new tool - e.g. flang/tools/driver and then make it a build-time configuration option which one to build in flang? That way we could bring the new driver up independently of the old driver and we would not be hitting this issue with the names. We would then not need this patch for the immediate issue we face today, although something similar may be needed to support the binary renaming case.

The other thing that has changed since the last round of review on this patch is that F18 is now part of the same project as clang, so perhaps we should be submitting both halves of this work at the same time.

I don't hold any of these views too strongly. I would be happy with the current approach or what @awarzynski proposes but have a mild preference for the current approach as I think it better addresses the binary renaming issue.

[awarzynski]

@richard.barton.arm @CarolineConcatto thank you both for clarification! @CarolineConcatto , if you prefer to go ahead with this approach - could you please address all the outstanding comments? I've left a few comments that I'd like addressed first, but nothing major.

Also, IIUC, the approach that we are taking here is via a _custom_ fortran frontend. I'm looking at ccc_gcc_name as reference, which I believe stands for custom c compiler, see:

• link1
• link2
• link3

I couldn't find any other documentation other than ^^^. Anyway, I think that it would be useful to make it clear that this flag points to either a _custom_ frontend (e.g. cfc_flang_name), or perhaps _alternative_ frontend (e.g. afc_flang_name). Currently the name is a bit too generic IMO (i.e. we want to make the intention for the flag very clear).

Last minor point - I think that this patch is more about _specfying_ rather than _searching_ flang frontend. It would be helpful to have this clarified in the commit message.

Thanks for working on this! :)

clang/test/Driver/flang/clang-driver-2-frontend01.f90

7 ↗

(On Diff #244635)

1. You make only one copy here :)

2. In this file %t should be sufficient (i.e. you don't need %t1).

3. IIUC, you don't need to copy here at all. Instead of using %basename_t.tmp1 below you could use alternative_fortran_frontend (or some other name).

I would prefer if we avoided copying in these tests. It's a step that complicates the test, yet doesn't add anything in terms of test coverage.

10 ↗

(On Diff #244635)

There's only one run line here, so --check-prefixes is not required here.

Could you please use the default label instead, i.e. CHECK?

clang/test/Driver/flang/flang-driver-2-frontend01.f90

7–8 ↗

(On Diff #244635)

IIUC, copying is not required here. This could be simplified as:

! RUN: %clang --driver-mode=flang -###  %s 2>&1 | FileCheck --check-prefixes=ALL %s

Also, this scenario is already tested in flang.90. I'd rather we didn't add this file.

clang/test/Driver/flang/flang-driver-2-frontend02.f90

2 ↗

(On Diff #244635)

This test is very similar to clang-driver-2-frontend01.f90 - the scenario tested here is already covered elsewhere and IMO this file can be removed from this patch.

[CarolineConcatto]

This patch updates the flag name from ffc-fortran-name to cfc-fortran-name,
where cfc stands for custom frontend compiler.

Moreover, the tests need pruning to test cfc-frotran-name flag output.
The tests check if the flang frontend being called is "flang" or the
one passed by the flag -cfc-fortran-name.

[CarolineConcatto]

clang/test/Driver/flang/clang-driver-2-frontend01.f90

10 ↗

(On Diff #244635)

Ok!

clang/test/Driver/flang/flang-driver-2-frontend01.f90

7–8 ↗

(On Diff #244635)

Ok, I agree with you. The other test is more complete.

clang/test/Driver/flang/flang-driver-2-frontend02.f90

2 ↗

(On Diff #244635)

Ok, it is I certain way, as we are only testing the flag being used and not the driver.

[awarzynski]

@CarolineConcatto thank you for the updates!

• It looks like you accidentally pushed two commits in one patch :) Could you please squash and resubmit? (this should clean-up the diff). Ta!
• Perhaps CFCGenericFlangName instead of GenericFortranFE? This would better correspond to the new flag and be consistent with https://github.com/llvm/llvm-project/blob/42a56bf63f699a620a57c34474510d9937ebf715/clang/lib/Driver/ToolChains/Gnu.cpp#L183.

Naming is hard! Otherwise, this is ready to land IMO.

clang/include/clang/Driver/Driver.h

220–224

CFCGenericFlangName?

315–319

getCFCGenericFlangName?

clang/include/clang/Driver/Options.td

267

custom fortran compiler instead, right? IIUC:

• this flag is to specify _custom fortran compiler_ (cfc)
• we add flang_nameat the end because it can be interpreted as flang's alternative name and we want to be consistent with ccc_gcc_name

clang/lib/Driver/Driver.cpp

1091–1096

There's only one, called cfc-flang-name ;-)

clang/test/Driver/flang/custom_frontend_flang.f90

6

FE is a bit enigmatic. Perhaphs ... invokes the custom fortran compiler given by ...?

10

This comment is out of date :)

[awarzynski]

clang/lib/Driver/ToolChains/Flang.cpp

70–77

[nit] Could this be customFlangName instead? I think that it would better reflect the meaning.

[CarolineConcatto]

Rename the flag and fix reviews comments

[CarolineConcatto]

I'll close this PR as we believe we will not need this flag for Flang driver.
There are some development happening in:
https://github.com/banach-space/llvm-project
for now.

[CarolineConcatto]

I'll close this PR as we believe we will not need this flag for Flang driver.
There are some development happening in:
https://github.com/banach-space/llvm-project
for now.

clang/include/clang/Driver/Driver.h

public:
   CC1ToolFunc CC1Main = nullptr;
 
 private:
   /// Raw target triple.
   std::string TargetTriple;
 
   /// Name to use when invoking gcc/g++.
   std::string CCCGenericGCCName;
 
+  /// Name to use when invoking a custom fortran compiler.
+  std::string CFCGenericFlangName;
+
   /// Name of configuration file if used.
   std::string ConfigFile;
 
   /// Allocator for string saver.
   llvm::BumpPtrAllocator Alloc;
 
   /// Object that stores strings read from configuration file.
   llvm::StringSaver Saver;
 
Driver(StringRef ClangExecutable, StringRef TargetTriple,
          DiagnosticsEngine &Diags,
          IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr);
 
   /// @name Accessors
   /// @{
 
   /// Name to use when invoking gcc/g++.
   const std::string &getCCCGenericGCCName() const { return CCCGenericGCCName; }
 
+  /// Name to use when invoking a custom fortran compiler.
+  const std::string &getCFCGenericFlangName() const { return CFCGenericFlangName; }
+
   const std::string &getConfigFile() const { return ConfigFile; }
 
   const llvm::opt::OptTable &getOpts() const { return getDriverOptTable(); }
 
   const DiagnosticsEngine &getDiags() const { return Diags; }
 
   llvm::vfs::FileSystem &getVFS() const { return *VFS; }
 
   bool getCheckInputsExist() const { return CheckInputsExist; }

clang/include/clang/Driver/Options.td

def driver_mode : Joined<["--"], "driver-mode=">, Group<internal_driver_Group>,
   Flags<[CoreOption, DriverOption, HelpHidden]>,
   HelpText<"Set the driver mode to either 'gcc', 'g++', 'cpp', or 'cl'">;
 def rsp_quoting : Joined<["--"], "rsp-quoting=">, Group<internal_driver_Group>,
   Flags<[CoreOption, DriverOption, HelpHidden]>,
   HelpText<"Set the rsp quoting to either 'posix', or 'windows'">;
 def ccc_gcc_name : Separate<["-"], "ccc-gcc-name">, InternalDriverOpt,
   HelpText<"Name for native GCC compiler">,
   MetaVarName<"<gcc-path>">;
+def cfc_flang_name : Separate<["-"], "cfc-flang-name">, InternalDriverOpt,
+  HelpText<"Name for a custom fortran compiler(cfc) for flang">;
 
 class InternalDebugOpt : Group<internal_debug_Group>,
   Flags<[DriverOption, HelpHidden, CoreOption]>;
 def ccc_install_dir : Separate<["-"], "ccc-install-dir">, InternalDebugOpt,
   HelpText<"Simulate installation in the given directory">;
 def ccc_print_phases : Flag<["-"], "ccc-print-phases">, InternalDebugOpt,
   HelpText<"Dump list of actions to perform">;
 def ccc_print_bindings : Flag<["-"], "ccc-print-bindings">, InternalDebugOpt,

clang/lib/Driver/Driver.cpp

Driver::Driver(StringRef ClangExecutable, StringRef TargetTriple,
                IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS)
     : Diags(Diags), VFS(std::move(VFS)), Mode(GCCMode),
       SaveTemps(SaveTempsNone), BitcodeEmbed(EmbedNone), LTOMode(LTOK_None),
       ClangExecutable(ClangExecutable), SysRoot(DEFAULT_SYSROOT),
       DriverTitle("clang LLVM compiler"), CCPrintOptionsFilename(nullptr),
       CCPrintHeadersFilename(nullptr), CCLogDiagnosticsFilename(nullptr),
       CCCPrintBindings(false), CCPrintOptions(false), CCPrintHeaders(false),
       CCLogDiagnostics(false), CCGenDiagnostics(false),
-      TargetTriple(TargetTriple), CCCGenericGCCName(""), Saver(Alloc),
-      CheckInputsExist(true), GenReproducer(false),
+      TargetTriple(TargetTriple), CCCGenericGCCName(""), CFCGenericFlangName(""),
+      Saver(Alloc), CheckInputsExist(true), GenReproducer(false),
       SuppressMissingInputWarning(false) {
   // Provide a sane fallback if no VFS is specified.
   if (!this->VFS)
     this->VFS = llvm::vfs::getRealFileSystem();
 
   Name = std::string(llvm::sys::path::filename(ClangExecutable));
   Dir = std::string(llvm::sys::path::parent_path(ClangExecutable));
   InstalledDir = Dir; // Provide a sensible default installed dir.
if (const Arg *A = Args.getLastArg(options::OPT_resource_dir))
     ResourceDir = A->getValue();
 
   if (const Arg *A = Args.getLastArg(options::OPT_save_temps_EQ)) {
     SaveTemps = llvm::StringSwitch<SaveTempsMode>(A->getValue())
                     .Case("cwd", SaveTempsCwd)
                     .Case("obj", SaveTempsObj)
                     .Default(SaveTempsCwd);
   }
 
+  // Extract -cfc arg.
+  if (const Arg *A = Args.getLastArg(options::OPT_cfc_flang_name))
+    CFCGenericFlangName = A->getValue();
+
   setLTOMode(Args);
 
   // Process -fembed-bitcode= flags.
   if (Arg *A = Args.getLastArg(options::OPT_fembed_bitcode_EQ)) {
     StringRef Name = A->getValue();
     unsigned Model = llvm::StringSwitch<unsigned>(Name)
         .Case("off", EmbedNone)
         .Case("all", EmbedBitcode)
         .Case("bitcode", EmbedBitcode)

clang/lib/Driver/ToolChains/Flang.cpp

 //===-- Flang.cpp - Flang+LLVM ToolChain Implementations --------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 
 #include "Flang.h"
 #include "CommonArgs.h"
 
 #include "clang/Driver/Options.h"
-
 #include <cassert>
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 void Flang::ConstructJob(Compilation &C, const JobAction &JA,
                          const InputInfo &Output, const InputInfoList &Inputs,
void Flang::ConstructJob(Compilation &C, const JobAction &JA,
   } else {
     assert(Output.isNothing() && "Invalid output.");
   }
 
   const InputInfo &Input = Inputs[0];
   assert(Input.isFilename() && "Invalid input.");
   CmdArgs.push_back(Input.getFilename());
 
   const auto& D = C.getDriver();
-  const char* Exec = Args.MakeArgString(D.GetProgramPath("flang", TC));
+  const std::string &customFlangName = D.getCFCGenericFlangName();
+  const char *FlangName;
+  if (!customFlangName.empty())
+    FlangName = customFlangName.c_str();
+  else FlangName = "flang";
+
+  const char *Exec = Args.MakeArgString(getToolChain().GetProgramPath(FlangName));
   C.addCommand(std::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));
 }
 
 Flang::Flang(const ToolChain &TC)
     : Tool("flang", "flang frontend", TC, RF_Full) {}
 
 Flang::~Flang() {}

clang/test/Driver/flang/custom_frontend_flang.f90

+! Check wich compiler's name for fortran should be invoked.
+
+! The flag -cfc-flang-name is passed by the driver.
+! The flag has preference over "flang" compiler.
+! Therefore the driver invokes the compiler given by the flag.
+
+! RUN: %clang --driver-mode=flang -cfc-flang-name alternative_fortran_compiler -### %s 2>&1 | FileCheck %s
+
+! The invocations should be with alternative_fortran_compiler -fc1.
+! CHECK: "{{[^"]*}}alternative_fortran_compiler" "-fc1"