Page MenuHomePhabricator

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

Authored by CarolineConcatto on Tue, Feb 4, 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 TranscriptTue, Feb 4, 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.Tue, Feb 4, 5:38 AM
DavidTruby requested changes to this revision.Tue, Feb 4, 6:58 AM

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

clang/lib/Driver/Driver.cpp
131–132

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.

This revision now requires changes to proceed.Tue, Feb 4, 6:58 AM
richard.barton.arm requested changes to this revision.Tue, Feb 4, 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
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
13

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.

15

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?

19

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 marked 5 inline comments as done.Tue, Feb 4, 1:23 PM
CarolineConcatto added inline comments.
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
131–132

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

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

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"

15

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.

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
15

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

LGTM but wait for someone else to approve

clang/lib/Driver/Driver.cpp
131–132

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.Fri, Feb 14, 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
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

Does %t1 not work again on this line?

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

No they shouldn't ?

clang/include/clang/Driver/Options.td
268

I prefer the new option name though - thanks.

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

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
10

Understood - thanks.