Page MenuHomePhabricator

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

Authored by CarolineConcatto on Feb 4 2020, 4:13 AM.

Details

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 <caroline.concatto@arm.com>

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2020, 4:13 AM
CarolineConcatto retitled this revision from [Clang] Add logic to search for flang frontend to [Clang] [Driver]Add logic to search for flang frontend.Feb 4 2020, 5:38 AM
DavidTruby requested changes to this revision.Feb 4 2020, 6:58 AM

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

clang/lib/Driver/Driver.cpp
131

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–77

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.

This revision now requires changes to proceed.Feb 4 2020, 6:58 AM
richard.barton.arm requested changes to this revision.Feb 4 2020, 10:59 AM

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
264

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

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

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

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

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 marked 5 inline comments as done.Feb 4 2020, 1:23 PM
CarolineConcatto added inline comments.
clang/include/clang/Driver/Options.td
264

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
131

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

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

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

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

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

clang/include/clang/Driver/Options.td
264

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

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 accepted this revision.Feb 13 2020, 5:23 AM
DavidTruby marked an inline comment as done.

LGTM but wait for someone else to approve

clang/lib/Driver/Driver.cpp
131

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

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

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

  • [Clang]Rename flag and update the tests
CarolineConcatto marked 4 inline comments as done.Feb 14 2020, 5:42 AM
CarolineConcatto marked 3 inline comments as done.

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
265

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
11

Does %t1 not work again on this line?

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

No they shouldn't ?

clang/include/clang/Driver/Options.td
265

I prefer the new option name though - thanks.

CarolineConcatto marked an inline comment as done.Feb 14 2020, 8:58 AM
CarolineConcatto added inline comments.
clang/test/Driver/flang/clang-driver-2-frontend01.f90
11

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.

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

Understood - thanks.

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

sscalpone added inline comments.Apr 28 2020, 8:27 AM
clang/test/Driver/flang/flang-driver-2-frontend01.f90
2

typo wich

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

Typo

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 added a comment.EditedApr 30 2020, 2:50 AM

@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.

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.

@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:

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
8
  1. You make only one copy here :)
  1. In this file %t should be sufficient (i.e. you don't need %t1).
  1. 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.

11

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
8–9

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
3

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 marked 3 inline comments as done.

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.

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

Ok!

clang/test/Driver/flang/flang-driver-2-frontend01.f90
8–9

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

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

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

@CarolineConcatto thank you for the updates!

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
264

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
1087–1092

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

clang/test/Driver/flang/custom_frontend_flang.f90
5 ↗(On Diff #261451)

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

9 ↗(On Diff #261451)

This comment is out of date :)

awarzynski added inline comments.May 4 2020, 3:14 AM
clang/lib/Driver/ToolChains/Flang.cpp
70–77

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

CarolineConcatto marked 7 inline comments as done.

Rename the flag and fix reviews comments

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 abandoned this revision.Jun 8 2020, 7:16 AM

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.