[PATCH] D132094: [profile] Create only prof header when no counters

[Ellis Hoag via Phabricator via llvm-commits]

ellis accepted this revision.
ellis added a comment.
This revision is now accepted and ready to land.

In D132094#3750842 <https://reviews.llvm.org/D132094#3750842>, @gulfem wrote:

> In D132094#3733666 <https://reviews.llvm.org/D132094#3733666>, @ellis wrote:
>
>> When using the `-fprofile-function-groups` option we could hit the case where one of the groups has no profiled functions. In that case it would be more surprising to not find a raw profile, rather than an empty raw profile. If we do go with option 1, maybe we could emit a warning explaining why no profile was emitted. Or if we go with option 2, maybe a warning would also be good to alert the user that it's an empty profile.
>
> What kind of warning do you have in mind @ellis? Are you suggesting to put some text like `Empty Profile` when we read a profile via `llvm-profdata show` (which happens to contain only a header)?

On second thought, when the user runs `llvm-profdata show` they will see that zero functions are instrumented, so I'm not sure a warning there is necessary.

LGTM after resolving comments!



================
Comment at: compiler-rt/test/profile/Posix/instrprof-empty-profile.c:5
+// RUN: mkdir -p %t.d
+// RUN: echo "src:other.c" | sed -e 's/\\/\\\\/g' > %t-file.list
+// RUN: %clang_profgen -fprofile-list=%t-file.list -o %t %s
----------------
I've have issues with using this `sed` command like this where it broke a test on Windows, but I think that was because my profile-list file had multiple lines. For this case, I'm not sure `sed` is necessary because there are no `\` characters. Let's remove it to simplify the line.


================
Comment at: compiler-rt/test/profile/Posix/instrprof-empty-profile.c:8
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: test -f %t.profraw
+
----------------
Is this test necessary if we are going to use the raw profile in the `llvm-profdata` command below anyway?


================
Comment at: compiler-rt/test/profile/Posix/instrprof-empty-profile.c:10-11
+
+// RUN: llvm-profdata show %t.profraw > %t.out
+// RUN: FileCheck %s --check-prefix=RAW-PROFILE-HEADER-ONLY < %t.out
+
----------------
I think it's simpler to use a pipe rather than creating a temporary file.

Also, it looks like this and the next `FileCheck` are checking the same output so we can reuse the same default prefix.


================
Comment at: compiler-rt/test/profile/Posix/instrprof-empty-profile.c:20
+// RAW-PROFILE-HEADER-ONLY: Instrumentation level: Front-end
+// RAW-PROFILE-HEADER-ONLY-NEXT: Total functions: 0
+// RAW-PROFILE-HEADER-ONLY-NEXT: Maximum function count: 0
----------------
I think this might be the only important line to check. Do we need to check the other lines too?


================
Comment at: compiler-rt/test/profile/Posix/instrprof-shared-empty-profile.test:8
+
+The test verifies concatenating profiles with only headers and no profile data and counters.
+"""
----------------
Oh nice, thanks for handling and testing this case! (The same comments from the previous test apply here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132094