[PATCH] D73951: [Clang] [Driver]Add logic to search for flang frontend

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@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 <https://github.com/llvm/llvm-project/blob/42a56bf63f699a620a57c34474510d9937ebf715/clang/lib/Driver/ToolChains/Gnu.cpp#L183>
- link2 <http://lists.llvm.org/pipermail/cfe-dev/2011-June/015622.html>
- link3 <http://clang-developers.42468.n3.nabble.com/ccc-gcc-name-equivalent-to-call-as-and-ld-directly-td3076391.html>

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! :)



================
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:7
+
+! Copy clang to two temporary file
+! t1 is the new frontend name.
----------------
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.


================
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:10
+! RUN: cp %clang %t1
+! RUN: %clang --driver-mode=flang -fortran-fe %basename_t.tmp1 -### %s 2>&1 | FileCheck --check-prefixes=ALL %s
+
----------------
richard.barton.arm wrote:
> CarolineConcatto wrote:
> > richard.barton.arm wrote:
> > > Does %t1 not work again on this line?
> > 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.
> Understood - thanks.
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`?


================
Comment at: clang/test/Driver/flang/flang-driver-2-frontend01.f90:7-8
+! Copy clang to a temporary file to be the driver name
+! RUN: cp %clang %t1
+! RUN: %t1 --driver-mode=flang -###  %s 2>&1 | FileCheck --check-prefixes=ALL %s
+
----------------
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.


================
Comment at: clang/test/Driver/flang/flang-driver-2-frontend02.f90:2
+! Check wich name of flang frontend is invoked by the driver
+
+! The flag -fortran-fe is passed by the driver.
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73951/new/

https://reviews.llvm.org/D73951