This is an archive of the discontinued LLVM Phabricator instance.

[PGO] IR level instrumentation of indirect call value profiling
ClosedPublic

Authored by xur on Jan 8 2016, 3:26 PM.

Details

Summary

This patch adds the instrumentation for indirect call value profiling. It just finds all the indirect call-sites and generates instrprof_value_profile intrinsic calls. A new opt level option -disable-vp is introduced to disable
the value profiling instrumentation.

This patch has dependence on http://reviews.llvm.org/D16015

Diff Detail

Event Timeline

xur updated this revision to Diff 44383.Jan 8 2016, 3:26 PM
xur retitled this revision from to [PGO] instrumentation for indirect call value profiling.
xur updated this object.
xur added reviewers: davidxl, silvas, bogner, betulb.
xur added a subscriber: llvm-commits.
davidxl edited edge metadata.Jan 8 2016, 3:32 PM

Please update the title to be '... IR level instrumentation of ..'

xur retitled this revision from [PGO] instrumentation for indirect call value profiling to [PGO] IR level instrumentation of indirect call value profiling.Jan 8 2016, 3:34 PM
xur edited edge metadata.

Please also add test cases.

lib/Transforms/Instrumentation/PGOInstrumentation.cpp
347

To make sure that instrumentation and lowering assign the callsite index in the same way, some refactoring is needed -- defined a Icall visitor class that can be shared across.

351

Fix format.

xur updated this revision to Diff 45031.Jan 15 2016, 1:26 PM

revised the patch based on the comments: using visitor class now.

betulb added inline comments.Jan 15 2016, 2:23 PM
lib/Transforms/Instrumentation/PGOInstrumentation.cpp
317

I do not like the variables to have names like "Get....". IndirectCallSiteVisitor would have been more appropriate.

345

The return should be on its own line. clang-format.

davidxl added inline comments.Jan 15 2016, 2:25 PM
lib/Transforms/Instrumentation/PGOInstrumentation.cpp
345

use it with -style LLVM

LGTM with the naming nits fixed.

lib/Transforms/Instrumentation/PGOInstrumentation.cpp
317

Maybe
PGOIndirectCallSiteVisitor.

xur added a subscriber: xur.Jan 15 2016, 4:26 PM

Thanks for the comments. I'll fix them.

-Rong

xur updated this revision to Diff 45286.Jan 19 2016, 11:29 AM

Name and format change based on the review comments.
Also add a new test case.

Thanks,

-Rong

vsk added a subscriber: vsk.Jan 19 2016, 5:54 PM

Thanks xur, sorry for jumping on this late. Two nits --

lib/Transforms/Instrumentation/PGOInstrumentation.cpp
324

Is the !CS needed? If it doesn't feel too cramped, maybe the conditional could be CS.getCalledFunction() || !CS.getCalledValue().

389

I don't think you need this for your test. Please use a separate commit for this.

xur marked 2 inline comments as done.Jan 20 2016, 11:47 AM
xur added inline comments.
lib/Transforms/Instrumentation/PGOInstrumentation.cpp
324

You are very right. !CS is not need as this is a call instruction. The condition should have simplified to your version too.

389

will do.

xur updated this revision to Diff 45426.Jan 20 2016, 11:51 AM
xur marked 2 inline comments as done.

Revised patch based on vsk's review comments.

Thanks,

-Rong

This revision was automatically updated to reflect the committed changes.