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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the updates, more comments inline. Also:

> The remarks printed miss carets.

Why and can you share an example?

> The parseOptimizationRemark, addDiagnosticArgs and printDiagnosticOptions functions created are identical to the ones used in Clang.

In which case we should seriously consider moving this code somewhere where it can be shared. If outside the scope of this change, please document in code that there's scope for re-use.



================
Comment at: flang/include/flang/Frontend/CodeGenOptions.h:72-118
+  enum class RemarkKind {
+    RK_Missing,            // Remark argument not present on the command line.
+    RK_Enabled,            // Remark enabled via '-Rgroup'.
+    RK_EnabledEverything,  // Remark enabled via '-Reverything'.
+    RK_Disabled,           // Remark disabled via '-Rno-group'.
+    RK_DisabledEverything, // Remark disabled via '-Rno-everything'.
+    RK_WithPattern,        // Remark pattern specified via '-Rgroup=regexp'.
----------------
>From what I can see, this has been borrowed almost verbatim from Clang: https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/clang/include/clang/Basic/CodeGenOptions.h#L331-L376.

This is fine (and common throughout the driver), but please document more. In particular:
* Highlight that ATM this code is identical that what Clang contains (and add a `TODO` to perhaps share with Clang at some point),
* Highlight that the list of options in Flang and Clang is _identical - it is really good that the interfaces in Clang and Flang are consistent. That's a good reason to re-use the logic from Clang.




================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:156-158
+/// Parse a remark command line argument. It may be missing, disabled/enabled by
+/// '-R[no-]group' or specified with a regular expression by '-Rgroup=regexp'.
+/// On top of that, it can be disabled/enabled globally by '-R[no-]everything'.
----------------
Could you give an example (and write a test ) for `-Rgroup=regexp`. Also, @kiranchandramohan , is this form actually needed? I am thinking that it's a complexity that could be avoided. It  could definitely simplify this patch.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:786
       parseShowColorsArgs(args, /*defaultDiagColor=*/false);
+  res.getDiagnosticOpts().ShowColors = res.getFrontendOpts().showColors;
 
----------------
victorkingi wrote:
> Apparently without forwarding the color option to the CompilerInvocation, flang doesn't print remark errors with color. Hence the need for this.
> Also, a question, why do we have 2 instances of DiagnosticsEngine, one in CompilerInvocation and the other passed as an argument?
> Apparently without forwarding the color option to the CompilerInvocation, flang doesn't print remark errors with color. Hence the need for this.

It is already "forwarded" here: https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/flang/lib/Frontend/CompilerInvocation.cpp#L117-L122. Could you explain why you need this change _here_?

> Also, a question, why do we have 2 instances of DiagnosticsEngine, one in CompilerInvocation and the other passed as an argument?

[[ https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/flang/tools/flang-driver/fc1_main.cpp#L37-L40 | One  ]] is for the driver itself, to generate diagnostics related to "driver errors" (e.g. option parsing errors). The [[ https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/flang/include/flang/Frontend/CompilerInstance.h#L64-L65 | other ]] belongs to `CompilerInstance` rather than `CompilerInvocation` and is used to report compilation errors (e.g. semantic or parsing errors). 


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:1070
   }
 
+  addDiagnosticArgs(args, clang::driver::options::OPT_R_Group,
----------------
Is this specifically for parsing remarks options? Please add a comment


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:927
 
+class StandaloneBackendConsumer : public llvm::DiagnosticHandler {
+
----------------
Why `StandaloneBackendConsumer`? Isn't this specifically for remarks? Also, could you document this class a bit?


================
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,
----------------
Why is this method needed and can it be tested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320