[PATCH] D29004: New OptimizationRemarkEmitter pass for MIR

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

I am a bit confused by the `isEnabled()` code. Is this meant as a common tool for frontends to configure filtering? I couldn't find any user in llvm before this patch, maybe I missed something? Shouldn't opt and clang have similar lines like the 3 lines you added to llc.cpp?



================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:10-12
+// Optimization diagnostic interfaces for machine passes.  It's packaged as an
+// analysis pass so that by using this service passes become dependent on MBFI
+// as well.  MBFI is used to compute the "hotness" of the diagnostic message.
----------------
Use `/// \file ...`


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:27
+/// \brief Common features for diagnostics dealing with optimization remarks
+/// that are used IR passes.
+class MachineOptimizationRemarkBase
----------------
**by machine** passes.


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:31
+public:
+  MachineOptimizationRemarkBase(enum DiagnosticKind Kind, const char *PassName,
+                                StringRef RemarkName, const DebugLoc &DLoc,
----------------
Side remarks that we should not fix in this commit (because it's the same for other classes):
- The base class is specifically designed to use an int for the diagnostic kind to stay extensible for plugins and then all the subclasses just go back to `enum DiagnosticKind`.
- DebugLoc is basically a pointer with special semantics so it is simpler and more efficient to pass it by value instead of `const DebugLoc&`.


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:43
+
+  MachineBasicBlock *getBlock() { return MBB; }
+
----------------
Could be `const`


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:127-128
+public:
+  MachineOptimizationRemarkEmitter(MachineFunction *MF,
+                                   MachineBlockFrequencyInfo *MBFI)
+      : MF(MF), MBFI(MBFI) {}
----------------
Use reference for MF and MBFI if they cannot be nullptr.


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:131
+
+  /// The new interface to emit remarks.
+  void emit(DiagnosticInfoOptimizationCommonBase &OptDiag);
----------------
`/// Emits an optimization remark` ?


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:142-143
+  bool allowExtraAnalysis() const {
+    // For now, only allow this with -fsave-optimization-record since the -Rpass
+    // options are handled in the front-end.
+    return MF->getFunction()->getContext().getDiagnosticsOutputFile();
----------------
Wouldn't a user of -Rpass not want false positives eliminated as well? I keep wondering why not just using `isEnabled()` to decide whether to do extra work or not.


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:169
+/// could be freed.
+class MachineOptimizationRemarkEmitterPass : public MachineFunctionPass {
+  std::unique_ptr<MachineOptimizationRemarkEmitter> ORE;
----------------
Wouldn't you expect the big majority of uses to be as an analysis pass? So maybe this could be merged with MachineOptimizationRemarkEmitter to keep things simpler? (We can always revert this decision when we find cases where we want a non-pass emitter).



================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:179
+
+  MachineOptimizationRemarkEmitter &getORE() {
+    assert(ORE && "pass not run yet");
----------------
Can be `const`


================
Comment at: lib/CodeGen/InlineSpiller.cpp:181
         TRI(*mf.getSubtarget().getRegisterInfo()),
-        MBFI(pass.getAnalysis<MachineBlockFrequencyInfo>()),
+        MBFI(pass.getAnalysis<MachineBlockFrequencyInfo>()), ORE(ORE),
         HSpiller(pass, mf, vrm) {}
----------------
Why not `pass.getAnalysis<MachineOptimizationRemarkEmitter>() instead of passing around the instance?


================
Comment at: lib/CodeGen/RegAllocGreedy.cpp:2639-2640
   DomTree = &getAnalysis<MachineDominatorTree>();
-  SpillerInstance.reset(createInlineSpiller(*this, *MF, *VRM));
+  auto *ORE = &getAnalysis<MachineOptimizationRemarkEmitterPass>().getORE();
+  SpillerInstance.reset(createInlineSpiller(*this, *MF, *VRM, ORE));
   Loops = &getAnalysis<MachineLoopInfo>();
----------------
I assume we do not need to create a new spiller instance each time when you change the spiller to do the getAnalysis<> (see above).


https://reviews.llvm.org/D29004