[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/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'.
----------------
awarzynski wrote:
> kiranchandramohan wrote:
> > awarzynski wrote:
> > > 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.
> > `Rpass=regex` is used, particularly `Rpass=.`. So this is required, but can be deferred to a separate patch to simplify this one.
> >  can be deferred to a separate patch to simplify this one
> 
> That would be a small win - the complexity comes from the fact that we can't rely on TableGen to define all possible options. 
Removed need for Rpass=regex 


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:786
       parseShowColorsArgs(args, /*defaultDiagColor=*/false);
+  res.getDiagnosticOpts().ShowColors = res.getFrontendOpts().showColors;
 
----------------
awarzynski wrote:
> 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). 
Thanks for the explanation, A bad regex error would be printed without color. But since we are getting rid of the regex option, might as well remove this.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:173-174
+    if (!result.Regex->isValid(regexError)) {
+      diags.Report(clang::diag::err_drv_optimization_remark_pattern)
+          << regexError << a->getAsString(args);
+      return false;
----------------
awarzynski wrote:
> Could you add a test to exercise this diagnostic?
added the tests in `optimization-remark.f90` These are the `-Rno-pass`, `-Rno-pass-analysis` and `-Rno-pass-missed` tests.


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:1007-1024
+  bool handleDiagnostics(const llvm::DiagnosticInfo &di) override {
+    switch (di.getKind()) {
+    case llvm::DK_OptimizationRemark:
+      optimizationRemarkHandler(llvm::cast<llvm::OptimizationRemark>(di));
+      break;
+    case llvm::DK_OptimizationRemarkMissed:
+      optimizationRemarkHandler(llvm::cast<llvm::OptimizationRemarkMissed>(di));
----------------
awarzynski wrote:
> Where is this method used?
`llvm/include/llvm/IR/DiagnosticHandler.h` specifies that this method needs to be overridden if one wants to setup custom diagnostic format reporting. `llvm/lib/IR/LLVMContext.cpp` then uses it for reporting in the diagnose function


```
void LLVMContext::diagnose(const DiagnosticInfo &DI) {
  if (auto *OptDiagBase = dyn_cast<DiagnosticInfoOptimizationBase>(&DI))
    if (LLVMRemarkStreamer *RS = getLLVMRemarkStreamer())
      RS->emit(*OptDiagBase);

  // If there is a report handler, use it.
  if (pImpl->DiagHandler &&
      (!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
      pImpl->DiagHandler->handleDiagnostics(DI))
    return;
  .
  .
  .
```


================
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,
----------------
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


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:169-171
+  clang::ProcessWarningOptions(flang->getDiagnostics(),
+                               flang->getDiagnosticOpts());
+
----------------
awarzynski wrote:
> Is this calling https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/include/clang/Basic/Diagnostic.h#L1840-L1842? That's part of the `clangBasic` library. The overall goal in the driver is to reduce the dependency on Clang and this would be a step in the opposite direction. IIUC, we only need a small subset of that function, right?
Yes, we only need a small subset. I'll create a static function in this file to avoid the dependence


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320