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

[victorkingi via Phabricator via cfe-commits]

victorkingi added inline comments.


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:976-1011
+  void
+  optimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationBase &d) {
+    if (d.isPassed()) {
+      // Optimization remarks are active only if the -Rpass flag has a regular
+      // expression that matches the name of the pass name in \p d.
+      if (codeGenOpts.OptimizationRemark.patternMatches(d.getPassName()))
+        emitOptimizationMessage(
----------------
awarzynski wrote:
> 
The if statement still needs to return if the pattern doesn't match, should I leave it the way it is?


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:30
 #include "llvm/Support/CommandLine.h"
+#include <clang/Basic/DiagnosticFrontend.h>
 
----------------
awarzynski wrote:
> No longer needed, right? Also, please use the the format consistent with the other `#include`s.
It's needed in the case of `clang::diag::warn_unknown_diag_option` in `emitUnknownDiagWarning` function.


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:117
+
+void processWarningOptions(clang::DiagnosticsEngine &diags,
+                           const clang::DiagnosticOptions &opts,
----------------
awarzynski wrote:
> 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.
I found it difficult including it in `CompilerInvocation.cpp` `parseCodeGenArgs` function since we are updating the DiagnosticsEngine object belonging to CompilerInstance not the other. We would have to expose the `DiagnosticsEngine` in `CompilerInvocation` class to do this. Would there be another way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320