This is an archive of the discontinued LLVM Phabricator instance.

[PGO]: Simplify coverage data lowering code
ClosedPublic

Authored by davidxl on Jan 3 2016, 3:58 PM.

Details

Reviewers
vsk
Summary

The names referenced by the coverage data may be associated with functions that are never emitted by Clang. That means those PGO names won't be materialized into the __llvm_prf_names section during instr-prof lowering. To make sure those names are emitted, the lowering pass will need to go through the coverage map global variable and check the referenced names.

The lowering code makes assumption about the layout of the coverage map and function record data which is error prone. Besides, in the near future, when name compression is implemented, the name references won't be available from the coverage data anymore.

In this patch, the referenced names are explicitly recorded in an internal global var and passed to the middle end. This simplifies the lowering code and also make it possible to do name compression. This is part-1 of the patch. Part-2 is in clang.

Diff Detail

Event Timeline

davidxl updated this revision to Diff 43866.Jan 3 2016, 3:58 PM
davidxl retitled this revision from to [PGO]: Simplify coverage data lowering code.
davidxl updated this object.
davidxl added a reviewer: vsk.
davidxl added a subscriber: llvm-commits.
vsk edited edge metadata.Jan 4 2016, 3:23 PM

I have a question regarding this:

Besides, in the near future, when name compression is implemented, the name references won't be available from the coverage data anymore.

Is the plan to change the FunctionRecord format by replacing Name with an offset into the CoverageNamesVar? If so, the temporary IR bloat of having a names array as well as complete function records would be OK.

With compression, the function record will have name MD5 replacing name pointer. The temporary IR increase will be small, but if we really care about it, we can always duplicate logic in clang such that only names for those functions clang decides to drop will be recorded.

vsk added a comment.Jan 5 2016, 8:34 AM

Ok. IIUC, when we replace name pointers with MD5 hashes, the function names will lose their uses. To keep lowerCoverageData working we'd need a solution like this. Lgtm (for parts 1 and 2).

I have updated the clang patch such that only unused function names will be passed through.

davidxl updated this revision to Diff 44235.Jan 7 2016, 11:11 AM
davidxl edited edge metadata.

Update comments. Further improve efficiency by skipping redundant map lookup (unused functions are guaranteed to be not in the map). With recent updates, the patch will now improve overall compile time with coverage mapping turned on.

vsk accepted this revision.Feb 24 2016, 6:22 PM
vsk edited edge metadata.

r257091

This revision is now accepted and ready to land.Feb 24 2016, 6:22 PM
vsk closed this revision.Feb 24 2016, 6:22 PM