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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
