[PATCH] D155452: [Flang] Add support for fsave-optimization-record

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for working on this, LG! I've left a few minor comments inline.



================
Comment at: clang/include/clang/Driver/Options.td:6449-6455
+def opt_record_file : Separate<["-"], "opt-record-file">, Flags<[FC1Option, CC1Option]>,
+  HelpText<"File name to use for YAML optimization record output">,
+  MarshallingInfoString<CodeGenOpts<"OptRecordFile">>;
+def opt_record_passes : Separate<["-"], "opt-record-passes">, Flags<[FC1Option, CC1Option]>,
+  HelpText<"Only record remark information for passes whose names match the given regular expression">;
+def opt_record_format : Separate<["-"], "opt-record-format">, Flags<[FC1Option, CC1Option]>,
+  HelpText<"The format used for serializing remarks (default: YAML)">;
----------------
Currently, these options have the following flags set:
*  `CC1Option` and `NoDriverOption`.
See:
* https://github.com/llvm/llvm-project/blob/5da317a79e3e53f17c6d356361807df1d16a0b66/clang/include/clang/Driver/Options.td#L6156

IIUC, you'd like this to be CC1Option` and `NoDriverOption` _and_ `FC1Option`. You can do that by moving these definitions here:
* https://github.com/llvm/llvm-project/blob/5da317a79e3e53f17c6d356361807df1d16a0b66/clang/include/clang/Driver/Options.td#L6810-L6835


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:195-197
+          args.getLastArg(clang::driver::options::OPT_opt_record_file)) {
+    opts.OptRecordFile = a->getValue();
+  }
----------------
Drop braces: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements. Same for the other options below.

Btw, I'd be OK with moving this to a dedicated hook for parsing e.g. "opt remark options". 


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:961-965
+  clang::DiagnosticsEngine &diags = ci.getDiagnostics();
+  const CodeGenOptions &codeGenOpts = ci.getInvocation().getCodeGenOpts();
+  Fortran::lower::LoweringOptions &loweringOpts =
+      ci.getInvocation().getLoweringOpts();
+
----------------
victorkingi wrote:
> Thought this refactoring might help making the function clearer. Let me know if its unnecessary
It's a nice clean-up!

I'd move it to a separate patch, but the noise level is low, so I am not too concerned. 


================
Comment at: flang/test/Driver/frontend-forwarding.f90:26
 
+! RUN: %flang -### %s 2>&1 \
+! RUN:     -foptimization-record-file=%t.opt.yaml \
----------------
Is a dedicated driver invocation needed for this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155452