[PATCH] D59268: [Remarks] Add -foptimization-record-passes to filter remark emission

[Jessica Paquette via Phabricator via llvm-commits]

paquette accepted this revision.
paquette added a comment.
This revision is now accepted and ready to land.

Awesome!

The code LGTM. I have a few minor nits, which are mostly personal taste.



================
Comment at: clang/include/clang/Driver/CC1Options.td:607
+def opt_record_passes : Separate<["-"], "opt-record-passes">,
+  HelpText<"Pass filter regex to use for the YAML optimization record output">;
 
----------------
This is kind of clunky IMO

Maybe like

"Only record remark information for passes whose names match the given regular expression"


================
Comment at: clang/include/clang/Driver/Options.td:1720
+  Group<f_Group>,
+  HelpText<"Specify the regex to filter the passes to be included in the generated optimization record">;
+
----------------
"Only include passes which match a specified regular expression in the generated optimization record"

Also, maybe we should make it clear that the default behaviour is to include all passes?


================
Comment at: clang/test/CodeGen/opt-record-MIR.c:27
 // YAML: DebugLoc:        { File: {{[^,]+}},
-// YAML:                    Line: 10,
+// YAML:                    Line: 12,
 // YAML:                    Column: 11 }
----------------
Not hugely critical, but can we avoid explicit line numbers in these tests? I think it's kind of flaky. It would be nice to have remarks tests be robust against unrelated changes as much as possible.

llvm/test/CodeGen/AArch64/machine-outliner-size-info.mir has an example of how you can combine the YAML output and standard remark output.


================
Comment at: llvm/lib/IR/RemarkStreamer.cpp:35-37
+  if (Optional<Regex> &Filter = PassFilter)
+    if (!Filter->match(Diag.getPassName()))
+      return;
----------------
Maybe we should keep track of the number of passes we matched? If it's 0, then we can emit a warning saying that we didn't match anything.

I can imagine people misspelling pass names/writing bad regexes etc and maybe wanting this information.

I guess that could be done in a follow-up though.


================
Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:91
+    cl::desc(
+        "Regex for the passes that need to be serialized to the output file."),
+    cl::value_desc("filename"));
----------------
Why is this cl::desc different from the others?


================
Comment at: llvm/tools/gold/gold-plugin.cpp:210
   static std::string OptRemarksFilename;
+  static std::string OptRemarksPasses;
   static bool OptRemarksWithHotness = false;
----------------
I feel like if I didn't know anything about this, I would assume that this defaults to all passes.

Maybe `OptRemarksFilter` would be a better name?


================
Comment at: llvm/tools/llc/llc.cpp:152
+static cl::opt<std::string> RemarksPasses(
+    "pass-remarks-passes",
+    cl::desc("Regex to decide which passes to include in the remarks"),
----------------
Maybe "-pass-remarks-only" or "pass-remarks-filter" instead of "passes"?

This is a matter of personal taste though, so I won't be offended if you leave it as is.


================
Comment at: llvm/tools/llc/llc.cpp:153
+    "pass-remarks-passes",
+    cl::desc("Regex to decide which passes to include in the remarks"),
+    cl::value_desc("regex"));
----------------
The other remarks descriptions all follow this format

"Enable optimization remarks from passes whose name match the given regular expression"

So maybe something like this would be more idiomatic

"Only record optimization remarks from passes whose names match the given regular expression"


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

https://reviews.llvm.org/D59268