[PATCH] D33514: [WIP] Bug 32352 - Provide a way for OptimizationRemarkEmitter::allowExtraAnalysis to check if (specific) remarks are enabled

[Adam Nemet via Phabricator via cfe-commits]

anemet added a comment.

Only minor things at this point.  This is very close now.



================
Comment at: include/llvm/Analysis/OptimizationDiagnosticInfo.h:81
   /// detected by the user.
-  bool allowExtraAnalysis() const {
-    // For now, only allow this with -fsave-optimization-record since the -Rpass
-    // options are handled in the front-end.
-    return F->getContext().getDiagnosticsOutputFile();
+  bool allowExtraAnalysis(StringRef &&PassName) const {
+    return (F->getContext().getDiagnosticsOutputFile() ||
----------------
Why rvalue reference, you should just take this by value.  There is no ownership transfer here.


================
Comment at: include/llvm/Analysis/OptimizationDiagnosticInfo.h:84
+            F->getContext().getDiagHandler()->isAnyRemarkEnabled(
+                std::move(PassName)));
   }
----------------
No std::move here.  You have more of this later.


================
Comment at: include/llvm/IR/DiagnosticHandler.h:13
+
+#include "llvm/Support/Regex.h"
+
----------------
I don't think you need this.


================
Comment at: include/llvm/IR/DiagnosticHandler.h:18
+
+struct DiagnosticHandler {
+public:
----------------
Please add a comment before the class.


================
Comment at: include/llvm/IR/DiagnosticHandler.h:19
+struct DiagnosticHandler {
+public:
+  void *DiagnosticContext = nullptr;
----------------
No need for public for struct.


================
Comment at: include/llvm/IR/DiagnosticHandler.h:27
+
+  /// DiagHandlerCallback is settable from the C API and base implimentation
+  /// of DiagnosticHandler will call it from handleDiagnostics(). Any derived
----------------
implementation 


================
Comment at: include/llvm/IR/DiagnosticHandler.h:45
+
+  /// checks if remark requested with -pass-remarks-analysis, override
+  /// to provide different implementation
----------------
Capitalize first word and end with period; comments are full sentences. 


================
Comment at: include/llvm/IR/DiagnosticHandler.h:57
+
+  /// checks if remark requested with -pass-remarks{-missed/-analysis}
+  bool isAnyRemarkEnabled(StringRef &&PassName) const {
----------------
I would drop the flag names here since if those are overridden this is not true.  Just say "Return true if any remark type is enabled."


================
Comment at: lib/IR/DiagnosticInfo.cpp:47-65
 /// \brief Regular expression corresponding to the value given in one of the
 /// -pass-remarks* command line flags. Passes whose name matches this regexp
 /// will emit a diagnostic via ORE->emit(...);
 struct PassRemarksOpt {
   std::shared_ptr<Regex> Pattern;
 
   void operator=(const std::string &Val) {
----------------
Is this still used here?


================
Comment at: lib/LTO/LTOCodeGenerator.cpp:667-668
-    return Context.setDiagnosticHandler(nullptr, nullptr);
-  // Register the LTOCodeGenerator stub in the LLVMContext to forward the
-  // diagnostic to the external DiagHandler.
-  Context.setDiagnosticHandler(LTOCodeGenerator::DiagnosticHandler, this,
----------------
I think that this comment still applies.


================
Comment at: tools/llvm-lto/llvm-lto.cpp:39-72
+static std::string CurrentActivity;
+namespace {
+struct LLVMLTODiagnosticHandler : public DiagnosticHandler {
+  bool handleDiagnostics(const DiagnosticInfo &DI) override {
+    raw_ostream &OS = errs();
+    OS << "llvm-lto: ";
+    switch (DI.getSeverity()) {
----------------
Don't move this code unless you have to.  The diff is easier to read that way.


https://reviews.llvm.org/D33514