[PATCH] D21771: [OptRemark] RFC: Add hotness attribute

[Adam Nemet via llvm-commits]

anemet added a comment.

In http://reviews.llvm.org/D21771#474604, @hfinkel wrote:

> Is there a reason that these should be free functions instead of just making an Analysis pass with these as member functions? Making it an actual analysis will make it easy to require BFI. I think the "get if available" will end up being confusing (sometimes the filtering will work, sometimes not, depending on what gets invalidated when - especially with the new pass manager where this will be dynamic). Side note: We might want to make BFI lazy (or compute-on-first-query instead of actually computing things in runOnFunction).


Thanks for feedback!

The main reason these are free function so that it's easy to transition from the current API which uses free function (i.e. no major reason ;-).  But yes, I wasn't completely happy with the BFI dependence either.  It was working for LV that already depends on BFI but it didn't work for LoopDist for example.  I added a new command flag and made the BFI dependence conditional on that.

How would an analysis pass help this.  It feels orthogonal because we can't unconditionally require the new analysis either because it would populate BFI.  I actually think that the new PM will help here because we could then hopefully check F.getEntryCount to see if PGO is available and only query BFI then, no?

Thinking a bit more, having this packaged as a new analysis pass may help because right now I have this in LoopDist for example:

+    if (PassRemarksWithHotness)      <------ this is the command line option
+      AU.addRequired<BlockFrequencyInfoWrapperPass>();

but when the emitOptimizationRemark* function is called, I don't know if BFI is actually available (i.e. whether the pass was updated with the change above) so I need to use getAnalysisIfAvailable.  Having the Analysis pass would take care of this because it's all under the control of the analysis pass.

Regarding the BFI population, I was thinking of using the command line flag (-pass-remarks-with-hotness).  Hopefully we can somehow detect if PGO is available and then enable the flag automatically at some higher level.  As you say, lazy BFI would be helpful but it would be nice not to directly depend on that feature for this project.

What do you think?

Adam


================
Comment at: include/llvm/IR/DiagnosticInfo.h:426
@@ -416,1 +425,3 @@
+  /// instrumentation run.  If profile informatin is not avaiable, this zero.
+  uint64_t Hotness;
 };
----------------
hfinkel wrote:
> I think that we should differentiate between 0 and "no information". Maybe make this an Optional<uint64_t>?
I was undecided about this but you moved the needle now :).  Will add.

================
Comment at: lib/Analysis/OptimizationDiagnosticInfo.cpp:27
@@ +26,3 @@
+
+  return BFI->getBFI().getBlockProfileCount(cast<BasicBlock>(V)).getValueOr(0);
+}
----------------
hfinkel wrote:
> V might not always be a BB. This is something you were planning to generalize later?
Yes, hence the cast so that we'd assert for anything else right.  I hope to refine this part of the design as more things get hooked up.


http://reviews.llvm.org/D21771