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

[Caroline via Phabricator via cfe-commits]

CarolineConcatto marked 5 inline comments as done.
CarolineConcatto added inline comments.


================
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">;
----------------
richard.barton.arm wrote:
> 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.
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


================
Comment at: clang/lib/Driver/Driver.cpp:131
       CCLogDiagnostics(false), CCGenDiagnostics(false),
-      TargetTriple(TargetTriple), CCCGenericGCCName(""), Saver(Alloc),
-      CheckInputsExist(true), GenReproducer(false),
+      TargetTriple(TargetTriple), CCCGenericGCCName(""), FFCGenericFortranName(""),
+      Saver(Alloc), CheckInputsExist(true), GenReproducer(false),
----------------
DavidTruby wrote:
> 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)
I tried to follow the pattern set by the other tools, like GNU.


================
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
----------------
richard.barton.arm wrote:
> 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.
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"


================
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
----------------
richard.barton.arm wrote:
> 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?
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.


================
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.
----------------
richard.barton.arm wrote:
> 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.
Good catch, it looks I've missed this text when re-writing the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73951