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

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.

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.



================
Comment at: clang/include/clang/Driver/Options.td:264
   MetaVarName<"<gcc-path>">;
+def fcc_fortran_name : Separate<["-"], "ffc-fortran-name">, InternalDriverOpt,
+  HelpText<"Name for native Fortran compiler">;
----------------
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.


================
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:12
+! t1 is the new frontend name.
+! RUN: cp %clang %t1
+! RUN: %clang --driver-mode=flang -ffc-fortran-name %basename_t.tmp1 -### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-OBJ %s
----------------
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.


================
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:14
+! RUN: %clang --driver-mode=flang -ffc-fortran-name %basename_t.tmp1 -### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-OBJ %s
+! CHECK-EMIT-OBJ-DAG: "-emit-obj"
+! CHECK-EMIT-OBJ-DAG: "-o" "{{[^"]*}}.o
----------------
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?


================
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:18
+! Should end in the input file.
+! ALL: "{{.*}}clang-driver-2-frontend01.f90"{{$}}
----------------
Similarly, I don't understand the need for this check on the output, although it would be correct.


================
Comment at: clang/test/Driver/flang/driver-2-frontend01.f90:3
+
+! Driver name is a randon name. It does not contain flag, flang or clang,
+! therefore the driver invokes flang FE.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73951