[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Some high level comments:

- The logic in `parseCodeGenArgs` in CompilerInvocation.cpp is a bit complex and quite specialized - could you move it to a dedicated method?
- In a fair few places this patch make references to "diagnostic flags" or "diagnostic options". That's borrowed from Clang where there's one code path for `-W<opt>` and `-R<opt>` flags. Note that in Clang the logic for `-W<opt>` is vastly more complex. This is completely absent in Flang. So, everything that's added here is added specifically for "remarks" and it would be helpful to be explicit about that.  For example, `processWarningOptions` is misleading. `processRemakrsOptions` would be more accurate.

Thank you :)



================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:227
 
+  // Specifies what passes to include. If not provided
+  // -fsave-optimization-record will include all passes.
----------------
> // Specifies what passes to include.

Could you be more specific, what passes are you referring to? Included where?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:242
+
+  // Specify which missed passes should be reported using a regex.
+  opts.OptimizationRemarkMissed = parseOptimizationRemark(
----------------
>   // Specify which missed passes should be reported using a regex.

Reported where?


================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:37
+// Print any diagnostic option information to a raw_ostream.
+static void printDiagnosticOptions(llvm::raw_ostream &os,
+                                   clang::DiagnosticsEngine::Level level,
----------------
victorkingi wrote:
> awarzynski wrote:
> > Why is this method needed and can it be tested?
> This method prints out the pass that was done e.g. [-Rpass=inline ], [-Rpass=loop-delete] next to the optimization message.
> It is tested by the full remark message emitted test in flang/test/Driver/optimization-remark.f90
Flang has very _very_ limited support for warning flags. So this is going to be used specifically for Remarks. Please update and document accordingly.


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:30
 #include "llvm/Support/CommandLine.h"
+#include <clang/Basic/DiagnosticFrontend.h>
 
----------------
No longer needed, right? Also, please use the the format consistent with the other `#include`s.


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:117
+
+void processWarningOptions(clang::DiagnosticsEngine &diags,
+                           const clang::DiagnosticOptions &opts,
----------------
awarzynski wrote:
> ?
I find this method quite confusing.

1. It doesn't process warning options - it processes remarks options (so the name is inaccurate).
2. It deals with `-Rpass -Rno-pass` cases (i.e. postive + negative flag), but that's normally done in CompilerInvocation when parsing other options. See e.g. https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/flang/lib/Frontend/CompilerInvocation.cpp#L161-L163. Why not use that logic instead?
3. It's been extracted from Clang - it would be very helpful to note that and highlight the difference between this method and similar method in Clang.
4. You can safely trim it to only deal with Remarks (so e.g. update `const clang::DiagnosticOptions &opts,` in the signature)

Now, I am not requesting any major refactor here - I appreciate that e.g. these remark flags are defined a bit differently to other flags. But this method can be simplified and should be documented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320