[PATCH] D29004: New OptimizationRemarkEmitter pass for MIR

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
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();
----------------
anemet wrote:
> MatzeB wrote:
> > 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.
> I think that the comments are misleading.  The idea of this flag is not to reduce the number of false positives but to be able to report cases where the false positive rate is expected to be high without some further analysis.
> 
> In other words if this flag is off (i.e. -Rpass) we won't report these cases at all.
> 
> Let me fix the comment in a separate commit.
Ah that makes sense. I guess even the term "false positive" feels a bit out of place for information like where/how much was spilled. As those aren't immediately actionable things but rather extra information / statistics (at least when viewed in isolation without having something to compare against).


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:169
+/// could be freed.
+class MachineOptimizationRemarkEmitterPass : public MachineFunctionPass {
+  std::unique_ptr<MachineOptimizationRemarkEmitter> ORE;
----------------
anemet wrote:
> MatzeB wrote:
> > 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).
> > 
> The other reason why this got split for LLVM-IR ORE was the new pass manager where you have now "two" passes (legacy, new) producing ORE.
> 
> I think I would prefer to keep this separation.
Ah I didn't think about the new pass manager, makes sense.


================
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) {}
----------------
anemet wrote:
> MatzeB wrote:
> > Why not `pass.getAnalysis<MachineOptimizationRemarkEmitter>() instead of passing around the instance?
> Because the InlineSpiller is used from other allocators as well.  I'll add a comment that explains this.
Sure it's used by multiple allocators, but I don't see why this would prohibit the use of getAnalysis<> here.


https://reviews.llvm.org/D29004