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

[Adam Nemet via Phabricator via llvm-commits]

anemet added a reviewer: davidxl.
anemet added a comment.

Thanks very much for working on this!  Please assign https://bugs.llvm.org/show_bug.cgi?id=33794 to you.  Also check out my note in the bug.

> Hotness information for remarks (L505 and L751) do not display hotness information, most likely due to profile information not being propagated yet. Unsure if this is the desired outcome.

You need to use -pass-remarks-with-hotness for that.



================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:163-183
+  bool emitAnnotations(Function &F, OptimizationRemarkEmitter &ORE);
+  ErrorOr<uint64_t> getInstWeight(const Instruction &I,
+                                  OptimizationRemarkEmitter &ORE);
+  ErrorOr<uint64_t> getBlockWeight(const BasicBlock *BB,
+                                   OptimizationRemarkEmitter &ORE);
   const FunctionSamples *findCalleeFunctionSamples(const Instruction &I) const;
   std::vector<const FunctionSamples *>
----------------
Rather than passing and ORE reference around like this, can't we make it a member to this class?


================
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()
----------------
Should these be OptimizationRemarkAnalysis instead?  These aren't really optimization performed (or missed) i.e. either negative or positive.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:506
+      ORE.emit(OptimizationRemark(DEBUG_TYPE, "AppliedSamples", &Inst)
+               << "Applied " << Twine(*R).str()
+               << " samples from profile (offset: " << Twine(LineOffset).str()
----------------
You probably want to stream this out as named-value argument.  Looks for NV in Inliner.cpp for an example.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:507-508
+               << "Applied " << Twine(*R).str()
+               << " samples from profile (offset: " << Twine(LineOffset).str()
+               << ((Discriminator) ? "." + Twine(Discriminator).str() : "") +
+                      ")");
----------------
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?


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:679-682
+  std::function<AssumptionCache &(Function &)> GetAssumptionCache =
+      [&](Function &F) -> AssumptionCache & {
+    return ACT->getAssumptionCache(F);
+  };
----------------
Formatting only?


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:751-753
+        ORE.emit(OptimizationRemark(DEBUG_TYPE, "HotInline", DLoc, BB)
+                 << "inlined hot callee '" << CalledFunction->getName().str()
+                 << "' into '" << F.getName() << "'");
----------------
Again see Inliner.cpp how to stream these out as named-value arguments.


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


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1492
 bool SampleProfileLoader::runOnFunction(Function &F) {
+  OptimizationRemarkEmitter ORE(&F);
   F.setEntryCount(0);
----------------
For the new pass manager path (when you enter via SampleProfileLoaderPass::run we shouldn't create ORE on the file but rather get it from FunctionAnalysisManagerModuleProxy.  See IndirectCallPromotion.cpp for an example.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1503-1504
 
-  SampleProfileLoader SampleLoader(
-      ProfileFileName.empty() ? SampleProfileFile : ProfileFileName);
+  SampleProfileLoader SampleLoader(ProfileFileName.empty() ? SampleProfileFile
+                                                           : ProfileFileName);
 
----------------
Commit formatting change in otherwise unmodified code separately.


================
Comment at: test/Transforms/SampleProfile/remarks.ll:30-40
+; Checking to see if YAML file is generated and contains remarks
+; YAML:  --- !Passed
+; YAML:  Pass:            sample-profile
+; YAML:  Name:            HotInline
+; YAML:  --- !Passed
+; YAML:  Pass:            sample-profile
+; YAML:  Name:            AppliedSamples
----------------
Please check the full records with YAML-NEXT.


Repository:
  rL LLVM

https://reviews.llvm.org/D36127