[PATCH] D42212: [ThinLTO] Add call edges' relative block frequency to per-module summary.

[Easwaran Raman via Phabricator via llvm-commits]

eraman marked 2 inline comments as done.
eraman added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:58
   HotnessType Hotness = HotnessType::Unknown;
+  uint64_t RelBlockFreq = 0;
 
----------------
tejohnson wrote:
> eraman wrote:
> > tejohnson wrote:
> > > Do we need 64 bits in practice? Especially since these are scaled down by the entry freq. Concerned about the size increase for large indices that have huge numbers of edges. Also, since we'll never have both the Hotness and the RelBlockFreq, it would be great if we could union them, although that would require a flag on the index and I suppose we might want to support mixing modules built with and without profile data? Perhaps some other way to combine them. I.e. use a single field and reserve the lowest 3 bits for the hotness, shifting the relative frequency into the upper n-3 bits...
> > From our offline discussions I thought the use of 64 bits doesn't matter as they get compressed. Is the concern about in-memory size? I can reduce the size to, say, 29 bits (uint32_t with 3 bits reserved for the enum). Another thing to note is that only the per-module summary has this information.  Is there a way to make use of this?
> Right - concerned about in-memory size. Why does only the per-module summary contain this? Isn't that just temporary - I thought the idea was to propagate this into the backends?
Made this into a 29 bit bitfield.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:63
+cl::opt<bool> WriteRelBFToSummary(
+    "write-relbf-to-summary", cl::Hidden, cl::init(false),
+    cl::desc("Write relative block frequency to function summary "));
----------------
tejohnson wrote:
> eraman wrote:
> > tejohnson wrote:
> > > Suggest moving this to the bitcode writer and always adding RelBlockFreq into the in-memory index here.
> > > 
> > > Is there any reason to default off? Why not just default on from the get-go?
> > Until this information actually gets used (first propagate the counts and then use the counts in the inliner), this just poses extra compile time and increased summary size if not turned off. Alternatively, I can drop this patch and then send it as part of a larger patch that builds the callgraph and propagates the counts.
> How long do you expect until the other patches to be sent? If not too long I don't know that it matters too much since we are going to incur the extra space/memory/time in the near term anyway. Better to have it tested.
Most of the patches are adding necessary infrastructure but the final component is tuning the inliner  which could take a long time. I have now changed the code to compute relative block frequency unconditionally but have left the default value of the flag to false so it won't be written. 


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:290
+          // being rounded down to 0.
+          BBFreq /= BFI->getEntryFreq();
+          CallGraphEdges[Index.getOrInsertValueInfo(
----------------
tejohnson wrote:
> Any chance getEntryFreq will be 0?
When they are computed they can't be 0, but there is an interface to set block frequency (this is used for incremental update) although in that case  BFI should be recalculated after the pass. In any case, since the possibility exists I have guarded the division. 


Repository:
  rL LLVM

https://reviews.llvm.org/D42212