[PATCH] D29004: New OptimizationRemarkEmitter pass for MIR

[Matthias Braun via Phabricator via llvm-commits]

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

LGTM, the issues mentioned can be fixed without further review.



================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:179
+
+  MachineOptimizationRemarkEmitter &getORE() {
+    assert(ORE && "pass not run yet");
----------------
anemet wrote:
> MatzeB wrote:
> > Can be `const`
> No, we need to call emit through this reference.
`getORE() const` should still be possible (`const MachineOptimizationRemarkEmitter&` is not possible of course)


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:9
+//===----------------------------------------------------------------------===//
+// \file
+// Optimization diagnostic interfaces for machine passes.  It's packaged as an
----------------
Needs three slashes.


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:154
+  /// available.
+  Optional<uint64_t> computeHotness(const MachineBasicBlock *MBB);
+
----------------
Use a reference for MBB parameter as it cannot be nullptr.


================
Comment at: lib/CodeGen/MachineOptimizationRemarkEmitter.cpp:9
+//===----------------------------------------------------------------------===//
+// \file
+// Optimization diagnostic interfaces for machine passes.  It's packaged as an
----------------
`/// \file ...`


================
Comment at: lib/CodeGen/MachineOptimizationRemarkEmitter.cpp:34-35
+    DiagnosticInfoMIROptimization &Remark) {
+  const MachineBasicBlock *MBB = Remark.getBlock();
+  if (MBB)
+    Remark.setHotness(computeHotness(MBB));
----------------
Could move the declaration into the if


================
Comment at: lib/CodeGen/RegAllocGreedy.cpp:430
+                                    unsigned &FoldedSpills);
+
+  /// Report the number of spills and reloads for each loop.
----------------
Would it make sense to add a `if (!OptimizationRemark::isEnabled(DEBUG_TYPE)) return;` here?


================
Comment at: lib/CodeGen/RegAllocGreedy.cpp:438
+
+    for (MachineLoop *L : *Loops)
+      reportNumberOfSplillsReloads(L, Reloads, FoldedReloads, Spills,
----------------
Shouldn't this do `Reloads = 0; FoldedReloads = 0; ...` in each loop iteration as we shouldn't add the values of an earlier loop to a later loop (we just want it from inner loops to outer loops I assume).


https://reviews.llvm.org/D29004