This is an archive of the discontinued LLVM Phabricator instance.

[CSSPGO] split context string III - llvm-profgen changes
ClosedPublic

Authored by hoy on Aug 19 2021, 7:19 PM.

Details

Reviewers
wmi
wenlei
wlei
Summary

This is the profile llvm-profgen part of the context split work. Please refer to D107299 for the original whole patch.

Diff Detail

Event Timeline

hoy created this revision.Aug 19 2021, 7:19 PM
hoy requested review of this revision.Aug 19 2021, 7:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 7:19 PM
wmi added inline comments.Aug 23 2021, 12:09 PM
llvm/tools/llvm-profgen/ProfiledBinary.h
124

I guess Offset2LocStackMap will serve as the immutable storage of all SampleContextStorageType. Is there way to enforce its immutability?

wmi added inline comments.Aug 23 2021, 12:10 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
335

Add a message.

348

Add a message.

hoy marked 2 inline comments as done.Aug 23 2021, 1:17 PM
hoy added inline comments.
llvm/tools/llvm-profgen/ProfiledBinary.h
124

Yes, it should be immutable once after symbolization is done. The checking of that may not be as easy as the CSName table in the reader. On the other hand, a std::unordered_map operation should not invalidate the values unless it removes or replaces the value. It should just work as other fields like the FuncStartAddrMap above. What do you think?

hoy updated this revision to Diff 368197.Aug 23 2021, 1:19 PM

Addressing Wei's comment.

wmi added inline comments.Aug 23 2021, 2:18 PM
llvm/tools/llvm-profgen/ProfiledBinary.h
124

Ah, ok, just find that iterators will be invalidated for unordered_map rehashing but pointers and references will still be valid. So it is fine.

hoy updated this revision to Diff 368680.Aug 25 2021, 10:42 AM

Updating D108437: [CSSPGO] split context string III - llvm-profgen changes

wmi accepted this revision.Aug 25 2021, 11:13 AM

LGTM.

llvm/include/llvm/MC/MCPseudoProbe.h
171

Add a comment to explain the uint32_t contains the pseudo index.

This revision is now accepted and ready to land.Aug 25 2021, 11:13 AM
wenlei added inline comments.Aug 26 2021, 10:45 PM
llvm/tools/llvm-profgen/CallContext.h
19–20

Does this using statement do anything?

llvm/tools/llvm-profgen/PerfReader.cpp
423

nit: NYI is confusing. This is simply not expected, instead of "Not yet implemented", right? we don't have a 3rd ContextKey type.

llvm/tools/llvm-profgen/ProfileGenerator.cpp
374

The name CallerContextRef probably is left over from before we renamed into SampleContextFrames. Rename CallerContextRef accordingly, same for other places. And all other places where Context is called Name better be renamed accordingly too.

llvm/tools/llvm-profgen/ProfiledBinary.cpp
423–424

emplace_back with variadic args and avoid temp Callsite

llvm/tools/llvm-profgen/ProfiledBinary.h
283

nit: emplace_back with variadic args

284

The actual string for Callsite.first is owned by MCPseudoProbeDecoder, right? Specifically, GUID2FuncDescMap.

hoy updated this revision to Diff 369361.Aug 29 2021, 7:09 PM
hoy marked an inline comment as done.

Address Wenlei's comment.

wenlei accepted this revision.Aug 30 2021, 6:24 PM

lgtm, thanks.

llvm/test/tools/llvm-profgen/recursion-compression-pseudoprobe.test
12

This order change is due to sortFuncProfiles change in D108435? Does the test change belong to this MD5 patch or D108435?

llvm/tools/llvm-profgen/ProfileGenerator.cpp
374

getFunctionProfileForLeafProbe has two instance of ContextRef still.

hoy added inline comments.Aug 30 2021, 6:39 PM
llvm/test/tools/llvm-profgen/recursion-compression-pseudoprobe.test
12

The order change is due to the flip of using > to < for sortFuncProfiles in D108433, so that we don't have to provide a > operator.

llvm/tools/llvm-profgen/CallContext.h
19–20

oops, looks like it's from a string replacement. Removed.

llvm/tools/llvm-profgen/PerfReader.cpp
423

Makes sense. Message changed.

llvm/tools/llvm-profgen/ProfileGenerator.cpp
374

Good catch. Renamed to CallerFrames.

374

Renamed to NewContext.

llvm/tools/llvm-profgen/ProfiledBinary.cpp
423–424

fixed

llvm/tools/llvm-profgen/ProfiledBinary.h
284

Yes, the string is owned by the probe decoder, unlike the strings returned from the symbolizer which have to be saved somewhere in the profiled binary.

wenlei added inline comments.Aug 30 2021, 6:53 PM
llvm/test/tools/llvm-profgen/recursion-compression-pseudoprobe.test
12

I was asking why this test change is here since the sortFuncProfiles is in the 2nd patch. But if you're going to commit these three patches together, it's not going to make a difference anyways.

hoy added inline comments.Aug 30 2021, 6:56 PM
llvm/test/tools/llvm-profgen/recursion-compression-pseudoprobe.test
12

Oh yeah, I'll commit all patches as a whole. Will update the original patch before checking them in.

hoy closed this revision.Aug 31 2021, 4:18 PM