This is an archive of the discontinued LLVM Phabricator instance.

{PGO] Profile annotation for MemOPSize value profile
ClosedPublic

Authored by xur on Mar 15 2017, 2:57 PM.

Details

Summary

This patch annotates the valuesites profile to memory intrinsic.
This is suggested in code review of D28966.

This patch has the dependence on https://reviews.llvm.org/D28965.

Note that I change the interface for annotateValueSite() InstrProf.h to write out all the available records the MaxMDCount is passed as 0.

Diff Detail

Repository
rL LLVM

Event Timeline

xur created this revision.Mar 15 2017, 2:57 PM
davidxl added inline comments.Mar 15 2017, 3:30 PM
lib/ProfileData/InstrProf.cpp
791 ↗(On Diff #91939)

more readable with

bool AllValues = (MDCount == 0);

..

if (!AllValues && --MDCount == 0)

break;
lib/Transforms/Instrumentation/PGOInstrumentation.cpp
1202 ↗(On Diff #91939)

the logic should be other way around:

if Kind == mem_op, do special thing.

On the other hand, why do we need to annotate all values? Shouldn't the transformation only care about top-N (say 3) values which pass certain threshold? All other values should be lumped together into the default value case.

xur marked an inline comment as done.Mar 15 2017, 4:03 PM
xur added inline comments.
lib/Transforms/Instrumentation/PGOInstrumentation.cpp
1202 ↗(On Diff #91939)

I was thinking indirect-call-promtion was special as other value profiles won't truncate the profile.

I could do the top-N values. But
(1) the range options was defined in another file.
(2) this slightly change the profile semantic: the default is for (-inf, PreciseRangeStart) and (PreciseRangeLast, LargeRangeValue). Inserting PreciseValue changes this.
(3) right now, the optimization generates a switch stmt. we might need more values for better switch optimization.

xur updated this revision to Diff 92075.Mar 16 2017, 3:31 PM

Integrate David's comments. Using a separated option to control the number of annotation for memop_size.

xur updated this revision to Diff 92076.Mar 16 2017, 3:32 PM

Fix the testcase.

davidxl added inline comments.Mar 17 2017, 10:50 AM
test/Transforms/PGOProfile/Inputs/memop_size_annotation.proftext
19 ↗(On Diff #92076)

can you make the data not sorted in the profile data ?

xur updated this revision to Diff 92169.Mar 17 2017, 11:01 AM
xur marked an inline comment as done.

randomize the order the VP profile entry in proftext, as suggested by David.

This revision is now accepted and ready to land.Mar 17 2017, 11:14 AM
This revision was automatically updated to reflect the committed changes.
vsk added inline comments.Mar 17 2017, 11:20 AM
lib/Transforms/Instrumentation/PGOInstrumentation.cpp
128 ↗(On Diff #92169)

Please add a space after "memop".

280 ↗(On Diff #92169)

I don't expect a function named "find" to annotate anything. Consider renaming this (maybe annotateMemIntrinsics)?