[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Hi @arnamoy10, thank you for working on this! Overall this looks good to me, but I'm wondering whether we could simplify it a bit? More comments inline.



================
Comment at: clang/include/clang/Driver/Options.td:4163
 defm d_lines_as_comments : BooleanFFlag<"d-lines-as-comments">, Group<gfortran_Group>;
-defm default_double_8 : BooleanFFlag<"default-double-8">, Group<gfortran_Group>;
-defm default_integer_8 : BooleanFFlag<"default-integer-8">, Group<gfortran_Group>;
-defm default_real_8 : BooleanFFlag<"default-real-8">, Group<gfortran_Group>;
+defm default_double_8 : BooleanFFlag<"default-double-8">, Group<gfortran_Group>, Flags<[FlangOption,FC1Option]>;
+defm default_integer_8 : BooleanFFlag<"default-integer-8">, Group<gfortran_Group>, Flags<[FlangOption,FC1Option]>;
----------------
Could you add a help text? Note that once you add a help text, you will have to add `FlangOnlyOption` flag to make sure this option doesn't show up in: 
```
clang -help
```

Some for other options here. Also, at that point, I suggest moving these options near other Flang options: https://github.com/llvm/llvm-project/blob/ec4fb5bcd3b92867156a5bd75fa0be4c74084f3c/clang/include/clang/Driver/Options.td#L4224-L4238.

AFAIK, these options are no longer forwarded to `gfortran` anyway (since this [[ https://github.com/llvm/llvm-project/commit/6a75496836ea14bcfd2f4b59d35a1cad4ac58cee | patch ]]).


================
Comment at: clang/include/clang/Driver/Options.td:4166
+defm default_real_8 : BooleanFFlag<"default-real-8">, Group<gfortran_Group>, Flags<[FlangOption,FC1Option]>;
+defm large_sizes : BooleanFFlag<"large-sizes">, Group<gfortran_Group>, Flags<[FlangOption,FC1Option]>;
 defm dollar_ok : BooleanFFlag<"dollar-ok">, Group<gfortran_Group>;
----------------
Is this option available in `gfortran`? If not we shouldn't be adding `Group<gfortran_Group>` here.


================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:22-28
+struct DialectOptions {
+    DialectOptions() {}
+    bool dfltReal{false};
+    bool dfltInt{false};
+    bool dfltDouble{false};
+    bool largeSizes{false};
+};
----------------
Do we really need this? if yes, `FrontendOptions.h` would probably be a better place for this.


================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:80-81
+  // Fortran Dialect options
+  std::unique_ptr<Fortran::common::IntrinsicTypeDefaultKinds> defaultKinds_;
+  Fortran::frontend::DialectOptions dialectOpts_;
+
----------------
Wouldn't it be possible to just use `defaultKinds_` here rather than `defaultKinds_` _and_ `dialectOpts_`? Do you think that we will need both?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:261
+           diags.getCustomDiagID(clang::DiagnosticsEngine::Error,
+             "Use of `-fdefault-double-8` requires `-fdefault-real-8`");
+           diags.Report(diagID);
----------------
Is this requirement document anywhere?


================
Comment at: flang/test/Flang-Driver/pipeline.f90:1-2
+! Test that flang can forward all of the flags which are documented as
+! being supported by gfortran to flang-new for a fortran input file.
+!
----------------
I suspect that this is C&P error from [[ https://github.com/llvm/llvm-project/blob/e6a62ac62571229d941dfe81affabdbc47e478eb/clang/test/Driver/gfortran.f90 | here ]] :)

IIUC, this file tests that the compiler driver (`flang-new`) forwards all Flang frontend options to the compiler frontend driver (`flang-new -fc1`) as expected. Could you update the description?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96344