On Mon, Mar 31, 2014 at 6:06 PM, Richard Smith <[email protected]> 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
