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

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.


================
Comment at: clang/include/clang/Driver/Driver.h:69
+    CLMode,
+    FlangMode,
   } Mode;
----------------
kiranchandramohan wrote:
> Is the comma by choice?
It was not. Fixed.


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:37
+  if (isa<AssembleJobAction>(JA)) {
+    CmdArgs.push_back("-emit-obj");
+  } else if (isa<PreprocessJobAction>(JA)) {
----------------
kiranchandramohan wrote:
> peterwaller-arm wrote:
> > richard.barton.arm wrote:
> > > 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?
> > I've removed them.
> Can it be removed from the Summary of this PR?
They have now been reintroduced after we found that it had an unpleasant failure mode. Better to implement them now, after all, and fail gracefully in the frontend.


================
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 ||
----------------
richard.barton.arm wrote:
> peterwaller-arm wrote:
> > richard.barton.arm wrote:
> > > 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?
> > In the RFC, the intent expressed was to mimick gfortran and clang options. I am making a spelling choice here that I think it should be called -fsyntax-only, which matches those. I intend to make the `flang -fc1` side match in the near future.
> > 
> > -fdebug-* could be supported through an `-Xflang ...` option to pass debug flags through.
> OK - happy with this approach. So -fsyntax-only and -emit-ast will be the new names for f18's -fdebug-semantics and -fdebug-dump-parse-tree I guess.
Agreed. They can still be changed later if necessary.


================
Comment at: clang/lib/Driver/Types.cpp:220
+
+  case TY_Fortran: case TY_PP_Fortran:
+    return true;
----------------
kiranchandramohan wrote:
> Now that there is a 2018 standard, I am assuming that f18 and F18 are also valid fortran extensions. Can these be added to the File types?
> 
> Also, should the TypeInfo file be extended to handle all Fortran file types? ./include/clang/Driver/Types.def
> 
> Also, should we capture some information about the standards from the filename extension?
My reading of the code is that both [fF]90 and [fF]95 both get turned into TY_Fortran/TY_PP_Fortran.

Both of these in the Types.def end up coerced to "f95", currently. I don't think the driver wants an entry in the Types.def for each fortran standard - it doesn't have them for other languages, for example, and I'm unsure what the benefit of that would be. The main purpose of that table as far as I see is to determine what phases need to run.

My feeling is that the name in the types table should be "fortran". I don't want to make that change in the scope of this PR, since it might be a breaking change. It looks to me like the name works its way out towards human eyeballs but is otherwise unused.

I would like to implement -std inference from the file extension and additional fortran filename extensions (f18 etc) in another change request.


================
Comment at: clang/lib/Driver/Types.cpp:220
+
+  case TY_Fortran: case TY_PP_Fortran:
+    return true;
----------------
richard.barton.arm wrote:
> peterwaller-arm wrote:
> > kiranchandramohan wrote:
> > > Now that there is a 2018 standard, I am assuming that f18 and F18 are also valid fortran extensions. Can these be added to the File types?
> > > 
> > > Also, should the TypeInfo file be extended to handle all Fortran file types? ./include/clang/Driver/Types.def
> > > 
> > > Also, should we capture some information about the standards from the filename extension?
> > My reading of the code is that both [fF]90 and [fF]95 both get turned into TY_Fortran/TY_PP_Fortran.
> > 
> > Both of these in the Types.def end up coerced to "f95", currently. I don't think the driver wants an entry in the Types.def for each fortran standard - it doesn't have them for other languages, for example, and I'm unsure what the benefit of that would be. The main purpose of that table as far as I see is to determine what phases need to run.
> > 
> > My feeling is that the name in the types table should be "fortran". I don't want to make that change in the scope of this PR, since it might be a breaking change. It looks to me like the name works its way out towards human eyeballs but is otherwise unused.
> > 
> > I would like to implement -std inference from the file extension and additional fortran filename extensions (f18 etc) in another change request.
> Agree with those first two, but that last one is surely a new feature that needs adding when such a feature is enabled in the rest of F18. 
> 
> We'd need to think that through carefully too. Classic flang never supported such an option and GCC's `-std` option from C/C++ does not work for Fortran. Also, would that go in the driver or in flang -fc1?
> 
> I suggest holding fire on this until we are ready to do it properly.
Agreed, thanks for clarifying.

I didn't intend to suggest that it was something to be implemented in the near future. Just the future.


================
Comment at: clang/test/Driver/fortran.f95:2
+! Check that the clang driver can invoke gcc to compile Fortran when in
+! --driver-mode=clang. This is legacy behaviour - see also --driver-mode=fortran.
 
----------------
richard.barton.arm wrote:
> Need to change "see also --drver-mode=fortran" to "--driver-mode=flang" here.
Fixed.


================
Comment at: clang/test/Driver/lit.local.cfg:1
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.f95',
+config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
                    '.cu', '.rs', '.cl', '.hip']
----------------
kiranchandramohan wrote:
> For completion .F95 also?
> Should we add f03,F03,f08,F08,f18,F18. May be not now.
Since the file I added was .F90, that is what got added.

I have added strictly what was used by the tests under the assumption that others could be introduced when those are used. I would prefer not to add the others until they exist and are exercised (perhaps could happen as part of -std inference).


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

https://reviews.llvm.org/D63607