[PATCH] D37085: [MachineOutliner] Add missed optimization remarks based off outliner cost model

[Adam Nemet via Phabricator via llvm-commits]

anemet added a comment.

You need a test ;)



================
Comment at: lib/CodeGen/MachineOutliner.cpp:45
 #include "llvm/ADT/Twine.h"
+#include "llvm/Analysis/OptimizationDiagnosticInfo.h"
+#include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
----------------
I don't think you need this.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:919
+        // that we don't have to emit them whenever we visit a machine function.
+        MachineOptimizationRemarkEmitter MORE(*MF, nullptr);
+        MachineOptimizationRemarkMissed R(DEBUG_TYPE, "NotOutliningCheaper",
----------------
This won't support hotness.  Apparently we never added a ctor for MORE that would populate MBFI internally.  Can you please add one?

It should take a MachineBranchProbabilityInfo in addition to MF but since that is an immutable pass you should be able to require it from here.  Then something like this should do the trick in the ctor:

  // First create a dominator tree.
  MachineDominatorTree MDT;
  MDT.getBase().recalculate(*MF);

  // Generate LoopInfo from it.
  MachineLoopInfo  MLI;
  MLI.getBase().analyze(MDT.getBase());

  // Finally calculate BFI.
  MBFI.calculate(*MF, MBPI, MLI);

I am fine if you want to do this in a follow-on.



================
Comment at: lib/CodeGen/MachineOutliner.cpp:923
+                                          MBB);
+        R << "Candidate " << NV("SequenceID", (uint64_t)NumCandidatesEvaluated)
+          << " [" << MNV("StartInstr", *C.first) << " ... "
----------------
The SequenceID feels like debugging output.  The goal here is to explain the user why an expected optimization not happening and not to help a compiler developer to debug the compiler.  That belongs more to DEBUG(dbgs() <<) messages.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:926
+          << MNV("EndInstr", *C.second)
+          << "] (Length: " << NV("Length", (uint64_t)StringLen)
+          << ") not outlined from " << NV("SourceFunction", MF->getName())
----------------
Just committed r311628 to add a ctor for NV taking unsigned long.  You shouldn't need the cast here.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:927
+          << "] (Length: " << NV("Length", (uint64_t)StringLen)
+          << ") not outlined from " << NV("SourceFunction", MF->getName())
+          << "; Outlining would take more instructions than not. "
----------------
The function is already present in the remark via the MBB above.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:928-932
+          << "; Outlining would take more instructions than not. "
+             " (OutliningCost: "
+          << NV("OutliningCost", (uint64_t)OutliningCost)
+          << ", NotOutliningCost: "
+          << NV("NotOutliningCost", (uint64_t)NotOutliningCost) << ")";
----------------
I would write this as:

<< "Cost of outlining (" << NV("OutliningCost", OutliningCost) << ") is higher than not outlining (" << NV("NotOutliningCost", NotOutliningCost) << ")"


https://reviews.llvm.org/D37085