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

[David Li via llvm-commits]

davidxl marked 8 inline comments as done.

================
Comment at: lib/profile/InstrProfilingFile.c:104
@@ +103,3 @@
+  if (ProfileFileSize < sizeof(__llvm_profile_header)) {
+    if (ProfileFileSize)
+      PROF_WARN("Unable to merge profile data: %s\n",
----------------
vsk wrote:
> Could we skip these two checks by moving the call to `__llvm_profile_check_compatibility` here?
yes

================
Comment at: lib/profile/InstrProfilingFile.c:149
@@ +148,3 @@
+  rc = doProfileMerging(ProfileFile);
+  if (rc || COMPILER_RT_FTRUNCATE(ProfileFile, 0L)) {
+    PROF_ERR("Profile Merging of file %s failed: %s\n", ProfileFileName,
----------------
vsk wrote:
> I think any successful call to `doProfileMerging` ought to be followed by COMPILER_RT_FTRUNCATE. Sink it into the function?
doProfileMerge just merges profile into memory, and the wrapper OpenFileForMerge does the rest to prepare for writing (including truncating). Another reason it is here is that doProfileMerge has multiple 'return 0' so extracting truncation code out is cleaner.

================
Comment at: lib/profile/InstrProfilingFile.c:245
@@ +244,3 @@
+        if (MergingEnabled || (FilenamePat[I] == 'm' && FilenamePat[I + 1]) ||
+            (FilenamePat[I + 1] == 'm' && FilenamePat[I + 2])) {
+          PROF_WARN(
----------------
vsk wrote:
> Should this check be: `if (MergingEnabled && (FilenamePat[I] == 'm' || FilenamePat[I + 1] == 'm'))`? Why check whether the character after the 'm' is nil?
The second error condition is that %m specifier is not at the end of the pat. This restriction is for convenience. I don't see a reason to support %[0-9]m in arbitrary locations.

================
Comment at: test/profile/instrprof-basic.c:18
@@ -5,2 +17,3 @@
+// RUN: %clang_profuse=%t.m.profdata -o - -S -emit-llvm %s | FileCheck %s --check-prefix=COMMON --check-prefix=MERGE
 
 int begin(int i) {
----------------
vsk wrote:
> Is the `lprofGetLoadModuleSignature` code path covered?
The plan is to have basic test coverage for now. More tests will be added as follow up as things stablizes.


http://reviews.llvm.org/D21056