This is an archive of the discontinued LLVM Phabricator instance.

[Coverage] Combine counts of expansion region if there are no code regions for the same area.
ClosedPublic

Authored by ikudrin on Apr 6 2016, 9:41 AM.

Diff Detail

Event Timeline

ikudrin updated this revision to Diff 52813.Apr 6 2016, 9:41 AM
ikudrin retitled this revision from to [Coverage] Use the count value of the outer region for an expansion region..
ikudrin updated this object.
ikudrin added reviewers: bogner, davidxl, vsk.
ikudrin added a subscriber: llvm-commits.

The motivation case for this patch is the following:

$ cat > sample.cpp << EOF
#define DO_LOOP1() \
    for (int K = 0; K < 2; ++K) {}

#define DO_LOOP2() \
    for (int K = 0; K < 2; ++K) {}

#define JUST_EXPAND() DO_LOOP2()

int main() {
    DO_LOOP1();
    JUST_EXPAND();
    return 0;
}
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping sample.cpp 
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata
       |    1|#define DO_LOOP1() \
      3|    2|    for (int K = 0; K < 2; ++K) {}
       |    3|
       |    4|#define DO_LOOP2() \
      3|    5|    for (int K = 0; K < 2; ++K) {}
       |    6|
      3|    7|#define JUST_EXPAND() DO_LOOP2()
       |    8|
      1|    9|int main() {
      3|   10|    DO_LOOP1();
      1|   11|    JUST_EXPAND();
      1|   12|    return 0;
      1|   13|}

As you may see, the counts for DO_LOOP macroses are 3, and these values are propagated to the outer code. As a result, counts for lines 7 and 10 are shown as 3 which looks quite weird.
After applying the patch, the output of llvm-cov looks more predictable:

$ llvm-cov show a.out -instr-profile default.profdata
       |    1|#define DO_LOOP1() \
      3|    2|    for (int K = 0; K < 2; ++K) {}
       |    3|
       |    4|#define DO_LOOP2() \
      3|    5|    for (int K = 0; K < 2; ++K) {}
       |    6|
      1|    7|#define JUST_EXPAND() DO_LOOP2()
       |    8|
      1|    9|int main() {
      1|   10|    DO_LOOP1();
      1|   11|    JUST_EXPAND();
      1|   12|    return 0;
      1|   13|}

Another example which is also fixed by this patch:

$ cat > sample.cpp << EOF
#define DO_LOOP() \
    for (int k = 0; k < 2; ++k) {}

#define DO_SOMETHING() \
    { \
        DO_LOOP(); \
    }

int main() {
    DO_SOMETHING();
    DO_SOMETHING();
    return 0;
}
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping sample.cpp 
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata
       |    1|#define DO_LOOP() \
      6|    2|    for (int k = 0; k < 2; ++k) {}
       |    3|
       |    4|#define DO_SOMETHING() \
      2|    5|    { \
      3|    6|        DO_LOOP(); \
      2|    7|    }
       |    8|
      1|    9|int main() {
      1|   10|    DO_SOMETHING();
      1|   11|    DO_SOMETHING();
      1|   12|    return 0;
      1|   13|}

Pay attention to line 6. The patched llvm-cov generates the following output:

$ llvm-cov show a.out -instr-profile default.profdata
       |    1|#define DO_LOOP() \
      6|    2|    for (int k = 0; k < 2; ++k) {}
       |    3|
       |    4|#define DO_SOMETHING() \
      2|    5|    { \
      2|    6|        DO_LOOP(); \
      2|    7|    }
       |    8|
      1|    9|int main() {
      1|   10|    DO_SOMETHING();
      1|   11|    DO_SOMETHING();
      1|   12|    return 0;
      1|   13|}
asl added a subscriber: asl.Apr 13 2016, 12:00 PM
davidxl edited edge metadata.Apr 20 2016, 11:59 AM

Your little motivating examples are great -- please add them as test cases as well.

llvm/trunk/lib/ProfileData/CoverageMapping.cpp
327

what is this change about?

410

Can you add a comment here with more explanation?

llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
526

Also add a test for "include".

ikudrin updated this revision to Diff 54669.Apr 22 2016, 9:29 AM
ikudrin edited edge metadata.
  • Reworked to correspond the changes of D18610.
  • Expanded the test.
  • Added some comments.

I'll add test cases for llvm-cov with the next update.

ikudrin updated this revision to Diff 54836.Apr 25 2016, 5:03 AM
  • Added a test for llvm-cov.

The code looks correct, but after a second though, I wonder if the logic in
fixExpansionRegion

should really be done in Clang: tools/clang/lib/CodeGen/CoverageMappingGen.cpp

The counter expression for the expansion region should really be the counter of the enclosing region, instead of 'enclosing region counter + ....'.

The code looks correct, but after a second though, I wonder if the logic in
fixExpansionRegion

should really be done in Clang: tools/clang/lib/CodeGen/CoverageMappingGen.cpp

The counter expression for the expansion region should really be the counter of the enclosing region, instead of 'enclosing region counter + ....'.

Thanks! Sounds reasonable, I'll see if the issue can be fixed there.

ikudrin updated this revision to Diff 55606.Apr 29 2016, 8:57 AM
ikudrin retitled this revision from [Coverage] Use the count value of the outer region for an expansion region. to [Coverage] Combine counts of expansion region if there are no code regions for the same area..
ikudrin updated this object.
  • To fully fix the original issue, D19725 is required.
  • Simplified the logic of combining regions.
  • Improved comments.
  • Reworked test cases to check only the issue with combining expansions.
vsk edited edge metadata.Apr 29 2016, 11:01 AM

Thanks, this is great!

unittests/ProfileData/CoverageMappingTest.cpp
455 ↗(On Diff #55606)

Could you use distinct counters here (e.g {2,3,5,7})? It would help show that the right regions are merged together.

davidxl accepted this revision.May 2 2016, 11:02 AM
davidxl edited edge metadata.

other than vsk's comments, lgtm.

lib/ProfileData/CoverageMapping.cpp
378 ↗(On Diff #55606)

Clarify that ExpansionRegions from expanding a macro nested in another macro will have the same source locations. For such a case, the expansion regions's counts need to be combined.

This revision is now accepted and ready to land.May 2 2016, 11:02 AM
This revision was automatically updated to reflect the committed changes.