[PATCH] D59514: [PGO] Profile guided code size optimization.

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi marked an inline comment as done.
yamauchi added inline comments.


================
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:335
+  assert(BFI);
+  if (!hasProfileSummary())
+    return false;
----------------
eraman wrote:
> This check is not needed as isFunctionColdInCallGraph first checks if summary is available.
> 
> I am not convinced this shouldOptimizeForSize belongs to ProfileSummaryInfo but I don't have any good suggestions. One possibility is to keep the flag in PassBuilder and pass it to individual passes which then check the boolean and isFunctionColdInCallGraph.  Or perhaps add a pass that annotates OptForSize attribute on functions based on profile data? (that will still not help with the BB version of this function). Thoughts?
> This check is not needed as isFunctionColdInCallGraph first checks if summary is available.

True. I'd prefer having it here to make it clearer that we only do this with PGO. But it can go either way here.

> I am not convinced this shouldOptimizeForSize belongs to ProfileSummaryInfo but I don't have any good suggestions. One possibility is to keep the flag in PassBuilder and pass it to individual passes which then check the boolean and isFunctionColdInCallGraph. Or perhaps add a pass that annotates OptForSize attribute on functions based on profile data? (that will still not help with the BB version of this function). Thoughts?

I agree - this may not belong to ProfileSummaryInfo. I think I put it there because it's tied to the existence of PSI and might potentially need access to some PSI-internal data or have opportunities for code sharing.

How about having a separate file like ProfileGuidedSizeOpt.h/cpp which hosts the flag and shouldOptimizeForSize, and more? I think that would have an advantage of being able to share the common code in a single place (calling isFunctionColdInCallGraph, etc.) and being less intrusive (not needing to change many pass constructors, etc.), and being able to handle the block-level profiles (unlike the OptForSize attribute).

I'm not sure which directory it could be in, as that would have an implication on the cross-directory dependencies (eg. if we want to use PSI/BB/MBB/BFI/MBFI as a parameter type in it, it must be somewhere above IR/Analysis/CodeGen yet accessible from Transform/CodeGen.). Is there some directory where this sort of files can be in?



Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59514/new/

https://reviews.llvm.org/D59514