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

[Vedant Kumar via llvm-commits]

vsk added a comment.

Thanks, comments inline --


================
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;
+
----------------
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.

================
Comment at: lib/ProfileData/ProfileSummary.cpp:36
@@ -34,1 +35,3 @@
 
+std::pair<Module *, std::unique_ptr<ProfileSummary>>
+    ProfileSummary::CachedSummary;
----------------
See comment above.

================
Comment at: lib/ProfileData/ProfileSummary.cpp:371
@@ +370,3 @@
+ProfileSummary *ProfileSummary::computeProfileSummary(Module *M) {
+  Metadata *MD = M->getProfileSummary();
+  if (!MD)
----------------
nit: I prefer `if (Metadata *MD = M->getProfileSummary()) return ...;`, but I'll leave it up to you.

================
Comment at: unittests/ProfileData/ProfileSummaryTest.cpp:23
@@ +22,3 @@
+
+  ProfileSummaryTest()
+      : IPS({100000, 900000, 999999}), SPS({100000, 900000, 999999}) {
----------------
I think this sort of initialization code is supposed to go into a `void SetUp()` method. Let gtest do its own thing with the test constructor.

================
Comment at: unittests/ProfileData/ProfileSummaryTest.cpp:52
@@ +51,3 @@
+
+  static bool compareSummary(ProfileSummary *PS1, ProfileSummary *PS2) {
+    if (PS1->getKind() != PS2->getKind())
----------------
It looks like this is worth lifting into a `friend bool operator==(...)` method for ProfileSummary. I think that would make things a bit easier to read; maybe clients would like to be able to compare summaries too. Wdyt?


http://reviews.llvm.org/D18622