[PATCH] D63607: [clang][driver] Add basic --driver-mode=fortran support for flang

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.
Herald added a subscriber: usaxena95.

Hi Peter

The overall approach seems good to me and matches how the driver is integrated in the original flang project so not too many surprises. I left a few comments mostly about the scope of the original patch. I wonder how much sense it makes to add support for routing options on to (f18) flang that it does not support yet? Would it not be better to add these options to the clang driver at the same time as they arrive in flang -fc1?

Ta
Rich



================
Comment at: clang/lib/Driver/Driver.cpp:4788
+bool Driver::ShouldUseFlangCompiler(const JobAction &JA) const {
+  // Say "no" if there is not exactly one input of a type flang understands.
+  if (JA.size() != 1 ||
----------------
This first clause surprised me. Is this a temporary measure or do we never intend to support compiling more than one fortran file at once?


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:37
+  if (isa<AssembleJobAction>(JA)) {
+    CmdArgs.push_back("-emit-obj");
+  } else if (isa<PreprocessJobAction>(JA)) {
----------------
F18 does not currently support these options that control the output like -emit-llvm and -emit-obj so this code doesn't do anything sensible at present. Would it not make more sense to add this later on once F18 or llvm/flang grows support for such options?


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:44
+    if (JA.getType() == types::TY_Nothing) {
+      CmdArgs.push_back("-fsyntax-only");
+    } else if (JA.getType() == types::TY_LLVM_IR ||
----------------
Looks like the F18 option spelling for this is -fparse-only? Or maybe -fdebug-semantics? I know the intention is to throw away the 'throwaway driver' in F18, so perhaps you are preparing to replace this option spelling in the throwaway driver with -fsyntax-only so this would highlight that perhaps adding the code here before the F18 driver is ready to accept it is not the right approach?


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:67
+  Args.AddAllArgs(CmdArgs, options::OPT_R_Group); // "Rpass-"" options passthrough.
+  Args.AddAllArgs(CmdArgs, options::OPT_gfortran_Group); // gfortran options passthrough.
+
----------------
Similarly to previous comment, do we want to be passing all gfortran options through to F18 in the immediate term or even the long term? I don't think there has been any agreement yet on what the options that F18 will support are (although I agree that gfortran-like options would be most sensible and in keeping with clang's philosophy)


================
Comment at: clang/test/Driver/fortran.f95:1
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran.
 
----------------
... when not in --driver-mode=fortran


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

https://reviews.llvm.org/D63607