This is an archive of the discontinued LLVM Phabricator instance.

[Profile] introduce interface __llvm_profile_dump
ClosedPublic

Authored by davidxl on Aug 2 2016, 11:55 PM.

Details

Summary

This new interface is intended to be used to collect profile in hand selected hot regions. It is a wrapper to __llvm_profile_write_file. The behavior of the latter (existing one) does not change.

Diff Detail

Event Timeline

davidxl updated this revision to Diff 66618.Aug 2 2016, 11:55 PM
davidxl retitled this revision from to [Profile] introduce interface __llvm_profile_dump.
davidxl updated this object.
davidxl added a reviewer: vsk.
davidxl added a subscriber: llvm-commits.
vsk edited edge metadata.Aug 3 2016, 11:03 AM

Neat! The test looks good.

lib/profile/InstrProfiling.c
81

Should probably also be cleared when llvm_prof_set_filename() or llvm_profile_merge_from_buffer() are called.

silvas added a subscriber: silvas.Aug 3 2016, 5:30 PM
silvas added inline comments.
lib/profile/InstrProfiling.h
135

I don't think it is clear to the user what "on-line profile merging is on" means. AFAIK, currently that implicitly gets turned on based on the filename.
Maybe it would be clearer to say that "the filename must contain a %m specifier"? (possibly in addition to what you already have).

silvas added a comment.Aug 3 2016, 5:31 PM

(other than that, this LGTM).

davidxl marked an inline comment as done.Aug 8 2016, 10:26 AM
davidxl added inline comments.
lib/profile/InstrProfiling.c
81

It is a mistake to not call reset counter before dumping again (due to double counting) regardless whether merging is used or not

davidxl updated this revision to Diff 67198.Aug 8 2016, 10:27 AM
davidxl edited edge metadata.

Updated comments according to review feedback

vsk added inline comments.Aug 8 2016, 10:38 AM
lib/profile/InstrProfiling.c
81

Oh, I see. This sequence of calls doesn't make sense: dump(), merge_profile_from_buffer(), dump(). You're saying you'd need to reset the counters some time before the second call to dump() anyway.

What about: dump(), set_filename(), dump()?. The second 'dump()' wouldn't go through, which seems like a problem.

This revision was automatically updated to reflect the committed changes.