[PATCH] D18622: Replace the use of MaxFunctionCount module flag

Vedant Kumar via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 30 14:53:58 PDT 2016


vsk added inline comments.

================
Comment at: include/llvm/ProfileData/ProfileCommon.h:75
@@ +74,3 @@
+  // Cache of last seen module and its profile summary.
+  static std::pair<Module *, std::unique_ptr<ProfileSummary>> CachedSummary;
+
----------------
eraman wrote:
> vsk wrote:
> > Hm, this won't work in multi-threaded environments.
> > 
> > You can either sink this into LLVMContext (preferred!), or make it an `llvm::ManagedStatic` defined outside of `ProfileSummary` for more visibility. If you go with the second option, you'll also need a mutex around all cache accesses.
> > 
> > Somewhere down the line you might consider making this cache some kind of (Small)?DenseMap.
> Yes indeed. Perhaps it is better to not cache this and instead let the clients cache the summary?
> 
> As to use of DenseMap, is this ever needed? I believe (and I may be wrong) the only place multiple modules are simultaneously present is in the LTO mode and there all modules are actually merged into one module which is where the IPO optimizations take place. 
> 
> Another crazy thought - we could just cache the ProfileSummary without any key since all modules compiled in a single process are bound to have the same profile summary (since summary is not module specific)
Sure, using a DenseMap is probably overkill. Having a one-entry cache seems useful though.

Regarding your last point: it's cheap to keep the `Module*` as a key, and keeping it makes the code less "magical" (no hidden assumptions about which Module you're referring to).


http://reviews.llvm.org/D18622





More information about the llvm-commits mailing list