[PATCH] D21056: [profile] in-process profile merging support Part-3

[Sean Silva via llvm-commits]

silvas added a comment.

Looks like Vedant covered most of the things I wanted to mention. LGTM with a couple nits.


================
Comment at: lib/profile/InstrProfilingFile.c:226
@@ +225,3 @@
+          (FilenamePat[I] >= '1' && FilenamePat[I] <= '9' &&
+           FilenamePat[I + 1] == 'm'));
+}
----------------
Naively, this seems like it may read off the end. A comment explaining why not would probably be helpful.

================
Comment at: lib/profile/InstrProfilingFile.c:296
@@ -170,1 +295,3 @@
  * filename with PID and hostname substitutions. */
+/* The length to hold uint64_t followed ty 2 digit pool id including '_' */
+#define SIGLEN 24
----------------
typo: `ty` should be `by`

================
Comment at: lib/profile/InstrProfilingInternal.h:160
@@ +159,3 @@
+/* Return the profile header 'signature' value associated with the current
+ * executation or shared library. The signature value can be used to for
+ * a profile name that is unique to this load module so that it does not
----------------
I think you meant "executable" instead of "executation".

================
Comment at: lib/profile/InstrProfilingMerge.c:24
@@ +23,3 @@
+uint64_t lprofGetLoadModuleSignature() {
+  /* A very fast way to compute the module signature.  */
+  uint64_t CounterSize = (uint64_t)(__llvm_profile_end_counters() -
----------------
I think it is more accurate to say "a module signature" instead of "the module signature". Ideally we would e.g. hash all the names or something else a bit more robust (but I think that just the sizes will be fine for now).


http://reviews.llvm.org/D21056