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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for trimming this, it's much easier to review! A few more suggestions, but nothing major.



================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:227-263
+  bool needLocTracking = false;
+
+  if (!opts.OptRecordFile.empty())
+    needLocTracking = true;
 
   if (const llvm::opt::Arg *a =
+          args.getLastArg(clang::driver::options::OPT_opt_record_passes)) {
----------------
```lang=cpp
  // coment
  if (const llvm::opt::Arg *a =
          args.getLastArg(clang::driver::options::OPT_opt_record_passes))
    opts.OptRecordPasses = a->getValue();

  // coment
  if (const llvm::opt::Arg *a =
          args.getLastArg(clang::driver::options::OPT_opt_record_format))
    opts.OptRecordFormat = a->getValue();

  // coment
  opts.OptimizationRemark = parseOptimizationRemark(
      diags, args, clang::driver::options::OPT_Rpass_EQ, "pass");

  // coment
  opts.OptimizationRemarkMissed = parseOptimizationRemark(
      diags, args, clang::driver::options::OPT_Rpass_missed_EQ, "pass-missed");

  // coment
  opts.OptimizationRemarkAnalysis = parseOptimizationRemark(
      diags, args, clang::driver::options::OPT_Rpass_analysis_EQ,
      "pass-analysis");

  if (opts.getDebugInfo() == llvm::codegenoptions::NoDebugInfo) {
    // If the user requested a flag that requires source locations available in
    // the backend, make sure that the backend tracks source location information.
    bool needLocTracking = !opts.OptRecordFile.empty() || opts.OptRecordPasses ||
                           !opts.OptRecordFormat.empty() ||
                           opts.OptimizationRemark.hasValidPattern() ||
                           opts.OptimizationRemarkMissed.hasValidPattern() ||
                           opts.OptimizationRemarkAnalysis.hasValidPattern();

    if (needLocTracking)
      opts.setDebugInfo(llvm::codegenoptions::LocTrackingOnly);
  }
```

I might have made typos when editing this, but hopefully the overall logic makes sense. Basically, I am suggesting that "option parsing" is separated from the logic for setting up the location tracking in the backend.


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:978-1005
+    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(
+            d, clang::diag::remark_fe_backend_optimization_remark);
+
----------------
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

```lang=cpp
  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(
            d, clang::diag::remark_fe_backend_optimization_remark);
      return;
    }

    if (d.isMissed()) {
      // Missed optimization remarks are active only if the -Rpass-missed
      // flag has a regular expression that matches the name of the pass
      // name in \p D.
      if (codeGenOpts.OptimizationRemarkMissed.patternMatches(d.getPassName()))
        emitOptimizationMessage(
            d, clang::diag::remark_fe_backend_optimization_remark_missed);
      return;
    }

    assert(d.isAnalysis() && "Unknown remark type");

    bool shouldAlwaysPrint = false;
    if (auto *ora = llvm::dyn_cast<llvm::OptimizationRemarkAnalysis>(&d))
      shouldAlwaysPrint = ora->shouldAlwaysPrint();

    if (shouldAlwaysPrint ||
        codeGenOpts.OptimizationRemarkAnalysis.patternMatches(
            d.getPassName()))
      emitOptimizationMessage(
          d, clang::diag::remark_fe_backend_optimization_remark_analysis);
  }
```


================
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));
----------------
Where is this method used?


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:1136
 
+  BackendRemarkConsumer m(diags, codeGenOpts);
+
----------------
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

> Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats).


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:169-171
+  clang::ProcessWarningOptions(flang->getDiagnostics(),
+                               flang->getDiagnosticOpts());
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320