[PATCH] Add support for optimization reports.

[Diego Novillo]

On Mon, Mar 31, 2014 at 6:06 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> ================
> Comment at: include/clang/Basic/DiagnosticGroups.td:665
> @@ -664,1 +664,2 @@
>  def RemarkBackendPlugin : DiagGroup<"remark-backend-plugin">;
> +def BackendOptimizationReport : DiagGroup<"Rpass">;
> ----------------
> Do you intend for this to be controlled by `-WRpass`? I don't think you should add a `DiagGroup` for this one at all.

No. I was monkeying similarly looking code. Is this why I'm getting
those '[-WRpass]' suffixes? I'll take it out.

> ================
> Comment at: include/clang/Driver/Options.td:257-258
> @@ -255,3 +256,4 @@
>  def Q : Flag<["-"], "Q">;
> -def R : Flag<["-"], "R">;
> +def Rpass_EQ : Joined<["-"], "Rpass=">, Group<R_Group>, Flags<[CC1Option]>,
> +  HelpText<"Report transformations performed by optimization passes">;
>  def S : Flag<["-"], "S">, Flags<[DriverOption,CC1Option]>, Group<Action_Group>,
> ----------------
> Should we have a `-Rno-pass=...` too?

Perhaps. Something like 'grep -v'?


> ================
> Comment at: lib/CodeGen/CodeGenAction.cpp:387-389
> @@ +386,5 @@
> +    SourceLocation Loc;
> +    // FIXME: This should be done earlier.
> +    Diags.setDiagnosticMapping(diag::remark_fe_backend_optimization_report,
> +                               diag::Mapping::MAP_REMARK, Loc);
> +    // FIXME: There should be a way of transferring the DILocation
> ----------------
> Maybe add a `DefaultRemark` to `Diagnostic.td`, and then add `DefaultRemark` to the `def` for your `Remark` to enable it by default, and drop this?

Thanks. I'm not sure I'm following you. Would your proposal enable the
diagnostic when -Rpass= is given?

>
> ================
> Comment at: lib/CodeGen/CodeGenAction.cpp:393-396
> @@ +392,6 @@
> +    // unnecessarily.
> +    DILocation DIL(D.getDebugLoc().getAsMDNode(D.getFunction().getContext()));
> +    StringRef FileName = DIL.getFilename();
> +    unsigned LineNum = DIL.getLineNumber();
> +    unsigned ColumnNum = DIL.getColumnNumber();
> +    Diags.Report(Loc, diag::remark_fe_backend_optimization_report)
> ----------------
> Yuck. :( It's not correct to rely on diagnostics being formatted this way: we support several output formats that don't use the `'file:line:col: '` formatting.
>
> Could we annotate the raw form of Clang's `SourceLocation` onto the IR so that we can use it here? Failing that, you can map the filename/line/column back to a `SourceLocation` by asking the `clang::SourceManager` nicely (see `SourceManager::translateFileLineCol` and `FileManager::getFile`).

You mean I can talk back to the FE from here? Excellent. I'll use
translateFileLineCol.


> ================
> Comment at: lib/CodeGen/CodeGenAction.cpp:397-399
> @@ +396,5 @@
> +    unsigned ColumnNum = DIL.getColumnNumber();
> +    Diags.Report(Loc, diag::remark_fe_backend_optimization_report)
> +        << Twine(FileName + ":" + Twine(LineNum) + ":" + Twine(ColumnNum) +
> +                 ": " + D.getMsg()).str();
> +  }
> ----------------
> I think this'll report the wrong warning flag, something like:
>
>   remark: something happened [-WRpass]

Yes. This is the #1 annoyance I reported in the original submission.

> Ideally we should report
>
>   remark: something happened [-Rpass=passname]
>
> You'll probably need to add an optional warning flag name override to `DiagnosticsEngine` to support this (and use it as an override in `printDiagnosticOptions` in `TextDiagnosticPrinter.cpp`).

OK, thanks for the pointer. I'll take a look.


Thanks. Diego.