[PATCH] D36127: [OptDiag] Updating Remarks in SampleProfile

[Tarun Rajendran via Phabricator via llvm-commits]

tarunr added a comment.

Thanks for reviewing and for pointing me to examples.

-pass-remarks-with-hotness doesn't seem to be helping me in getting hotness information in these remarks.



================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:505
     if (FirstMark) {
-      const Function *F = Inst.getParent()->getParent();
-      LLVMContext &Ctx = F->getContext();
-      emitOptimizationRemark(
-          Ctx, DEBUG_TYPE, *F, DLoc,
-          Twine("Applied ") + Twine(*R) +
-              " samples from profile (offset: " + Twine(LineOffset) +
-              ((Discriminator) ? Twine(".") + Twine(Discriminator) : "") + ")");
+      ORE.emit(OptimizationRemark(DEBUG_TYPE, "AppliedSamples", &Inst)
+               << "Applied " << Twine(*R).str()
----------------
anemet wrote:
> Should these be OptimizationRemarkAnalysis instead?  These aren't really optimization performed (or missed) i.e. either negative or positive.
Yes, I think that this makes sense


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:507-508
+               << "Applied " << Twine(*R).str()
+               << " samples from profile (offset: " << Twine(LineOffset).str()
+               << ((Discriminator) ? "." + Twine(Discriminator).str() : "") +
+                      ")");
----------------
anemet wrote:
> I don't know much about the internals of sample-based profiling but is LineOffset or Discriminator actionable to the user?  I.e. isn't this just a debugging aid if something goes wrong?
The LineOffset and Discriminator may be useful for the user to better determine how the compiler applied the profile to the source. Though, I agree it doesn't seem very actionable to the user 


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1259-1265
+      ORE.emit(OptimizationRemark(DEBUG_TYPE, "PopularDest", MaxDestInstr)
+               << "most popular destination for conditional branches at "
+               << ((BranchLoc) ? Twine(BranchLoc->getFilename() + ":" +
+                                       Twine(BranchLoc.getLine()) + ":" +
+                                       Twine(BranchLoc.getCol()))
+                                     .str()
+                               : Twine("<UNKNOWN LOCATION>").str()));
----------------
anemet wrote:
> You can stream out the instruction like we stream them out I think in LICM as a named-value argument.  This may include the LLVM IR name of the instruction which may or may not work in this case, so you may need to tweak DiagnosticInfoOptimizationBase::Argument to do the right thing here.
Currently it looks like the opcode of the instruction gets streamed out in DiagnosticInfoOptimizationBase::Argument. Do you mean that the original source code of the instruction or location of the instruction should be streamed out instead?


Repository:
  rL LLVM

https://reviews.llvm.org/D36127