This is an archive of the discontinued LLVM Phabricator instance.

Annotate timeline in Instruments with passes and other timed regions.
ClosedPublic

Authored by dsanders on Oct 5 2018, 3:56 PM.

Details

Summary

Instruments is a useful tool for finding performance issues in LLVM but it can
be difficult to identify regions of interest on the timeline that we can use
to filter the profiler or allocations instrument. Xcode 10 and the latest
macOS/iOS/etc. added support for the os_signpost() API which allows us to
annotate the timeline with information that's meaningful to LLVM.

This patch causes timer start and end events to emit signposts. When used with
-time-passes, this causes the passes to be annotated on the Instruments timeline.
In addition to visually showing the duration of passes on the timeline, it also
allows us to filter the profile and allocations instrument down to an individual
pass allowing us to find the issues within that pass without being drowned out
by the noise from other parts of the compiler.

Using this in conjunction with the Time Profiler (in high frequency mode) and
the Allocations instrument is how I found the SparseBitVector that should have
been a BitVector and the DenseMap that could be replaced by a sorted vector a
couple months ago. I added NamedRegionTimers to TableGen and used the resulting
annotations to identify the slow portions of the Register Info Emitter. Some of
these were placed according to educated guesses while others were placed
according to hot functions from a previous profile. From there I filtered the
profile to a slow portion and the aforementioned issues stood out in the
profile.

To use this feature enable LLVM_SUPPORT_XCODE_SIGNPOSTS in CMake and run the
compiler under Instruments with -time-passes like so:

instruments -t 'Time Profiler' bin/llc -time-passes -o - input.ll'

Then open the resulting trace in Instruments.

There was a talk at WWDC 2018 that explained the feature which can be found at
https://developer.apple.com/videos/play/wwdc2018/405/ if you'd like to know
more about it.

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders created this revision.Oct 5 2018, 3:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 12:21 PM
bogner accepted this revision.Feb 13 2019, 4:34 PM

Is there a more generic term for this than "signpost"? If so, it might be nice to use that to avoid confusion when/if other implementations show up. If not, that's fine.

Overall, looks reasonable to me, other than a few nitpicks.

cmake/modules/HandleLLVMOptions.cmake
893 ↗(On Diff #168551)

Can we detect when this is available rather than make it an option?

include/llvm/Support/Timer.h
16 ↗(On Diff #168551)

Public headers can't include config.h. We don't install it, so folks using llvm as a library can't see it. That said, do we even need this include?

lib/Support/Signposts.cpp
84–87 ↗(On Diff #168551)

Better to do #if/#else here, in case any compilers warn about the redundant assignment (I'm not sure if they do)

98–99 ↗(On Diff #168551)

Can Impl ever be null if HAVE_ANY_SIGNPOST_IMPL? I think we can remove these checks.

This revision is now accepted and ready to land.Feb 13 2019, 4:34 PM
dsanders updated this revision to Diff 187275.Feb 18 2019, 2:47 PM
dsanders marked 5 inline comments as done.

Updated before commit

Is there a more generic term for this than "signpost"? If so, it might be nice to use that to avoid confusion when/if other implementations show up. If not, that's fine.

ProfilerAnnotations would fit, it's possibly too generic though

cmake/modules/HandleLLVMOptions.cmake
893 ↗(On Diff #168551)

We can test for os_signpost_interval_begin() and only offer it when it's available but I think it should be optional. The os_signpost family are very cheap (mostly due to the constant literal string requirements) but they do have a run-time cost.

lib/Support/Signposts.cpp
98–99 ↗(On Diff #168551)

It shouldn't be possible. I'll remove them

bogner added inline comments.Feb 18 2019, 3:16 PM
cmake/modules/HandleLLVMOptions.cmake
893 ↗(On Diff #168551)

Makes sense, though I think defaulting the option to on when it's available makes sense. It may also make sense to default this to match asserts and allow it to be forced on or off (that's done elsewhere, IIRC)

dsanders marked an inline comment as done.Feb 19 2019, 10:18 AM
dsanders added inline comments.
cmake/modules/HandleLLVMOptions.cmake
893 ↗(On Diff #168551)

I've gone with the WITH_ASSERTS mechanism like ABI-breaking checks

This revision was automatically updated to reflect the committed changes.