[PATCH] D29004: New OptimizationRemarkEmitter pass for MIR

[Adam Nemet via Phabricator via llvm-commits]

anemet added a comment.

In https://reviews.llvm.org/D29004#653623, @MatzeB wrote:

> 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?


So backend users can use the default diagnostic handler (like opt) or define their own (like llc and clang).  The default handler is in  LLVMContext::diagnose which has similar code via isDiagnosticEnabled.



================
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();
----------------
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.


================
Comment at: include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h:169
+/// could be freed.
+class MachineOptimizationRemarkEmitterPass : public MachineFunctionPass {
+  std::unique_ptr<MachineOptimizationRemarkEmitter> ORE;
----------------
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.


================
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) {}
----------------
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.


https://reviews.llvm.org/D29004