[PATCH] D79920: [llvm][NFC] Simplify ProfileSummaryInfo state transitions.

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi added a comment.

I still think it'd be best to have the pure formatting changes (clang-format) in a separate patch.



================
Comment at: llvm/include/llvm/Analysis/ProfileSummaryInfo.h:63
+  /// If no samples were present, attempt to refresh.
+  void refresh();
 
----------------
mtrofin wrote:
> yamauchi wrote:
> > refresh() won't read again once once a non-null summary is created.
> > 
> > Do we anticipate a situation where the profile summary in the IR may be set once and then updated later perhaps conditionally? Who should call refresh?
> That's currently the case, too - see the old computeSummary() def and uses. 
> 
> Re. the second question (if we anticipate a situation...) - my goal with this patch is to help make such analysis simpler, by clarifying where the transitions happen. When such a scenario appears, the agent invalidating the profile should trigger the refresh.
> 
> I think, ideally, we'd control the state of the PSI from the pass pipeline through explicit pass calls - it'd make it even more clear what the state is at any given moment. This patch is a step in that direction.
I think there are two approaches here:

1. We have a single global point where PSI is filled in. (this might potentially require coordination between different parts of the code.)

2. We can have multiple points during the pass pipeline where PSI is filled in or updated. (just update PSI as we go and change the metadata.)

Which are we going with?

If 1, would it be useful to make calling refresh again after it's filled into an assert error? Currently it gets dropped silently, but it would catch if someone updates the PS in the IR and tries to update the PSI, but not really updating it.

If 2, would it be useful to make refresh update PSI every time it gets called? (which we could do for 1 as well?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79920