This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profdata] ProfileReader cleanup - preparation for MD5 refactoring - 2
ClosedPublic

Authored by huangjd on Apr 20 2023, 8:44 PM.

Details

Summary

Cleanup profile reader classes to prepare for complex refactoring as propsed in D147740, continuing D148868
This is patch 2/n. This patch refactors CSNameTable and related things

The decision to move CSNameTable up to the base class is because a planned improvement (D147740) to use MD5 to lookup Functions/Context frames. In this case we want a unified data structure between contextless function or Context frames, so that it can be mapped by MD5 value. Since Context Frames can represent contextless functions, it is being used for MD5 lookup, therefore exposing it to the base class

Diff Detail

Unit TestsFailed

Event Timeline

huangjd created this revision.Apr 20 2023, 8:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 8:44 PM
huangjd requested review of this revision.Apr 20 2023, 8:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 8:44 PM

Looks like a good cleanup. CC wenlei and hoy for a closer look.

hoy added inline comments.Apr 27 2023, 5:30 PM
llvm/include/llvm/ProfileData/SampleProfReader.h
690

Are we planning to retire SampleProfileReaderCompactBinary? The functions and fields being refactored are not supposed to be available for that class.

Cleanup profile reader classes to prepare for complex refactoring as propsed in D147740, continuing D148868

Might be a bit of nitpicking, but thought I'd mentioned it since it's related to earlier comment in another patch (D148876) about explaining "why" in change description - here I'd appreciate if you can explain why you need "CSNameTable and related things" to be moved up in the type hierarchy, and how that helps to prepare for the change to improve FDO build speed.

My understanding is that since you need to use MD5 as key for the profile map, everything related to MD5, names, CSnames need to be exposed as the top level interface. It's useful to others if you articulate the why explicitly upfront and the same goes for D148868.

llvm/include/llvm/ProfileData/SampleProfReader.h
690

Well... one more reason to remove compact binary support first before this refactoring. As pointed out in the other patch, compact binary is effectively deprecated, and new features are not supported with compact binary. So it creates a dilemma with refactoring as deduplication by hosting would move things that don't actually apply to compact binary to its parent class.

huangjd updated this revision to Diff 519334.May 3 2023, 7:18 PM

Rebase after compact binary deprecated

huangjd edited the summary of this revision. (Show Details)May 3 2023, 7:24 PM
snehasish accepted this revision.May 8 2023, 9:50 AM

LGTM, please wait a bit in case others have any concerns.

llvm/lib/ProfileData/SampleProfReader.cpp
564

Can we drop the else (and indentation for the block below) since this part is unconditional after the early return?

This revision is now accepted and ready to land.May 8 2023, 9:50 AM
wenlei accepted this revision.May 8 2023, 1:14 PM

lgtm, thanks.

huangjd updated this revision to Diff 520582.May 8 2023, 9:37 PM
huangjd edited the summary of this revision. (Show Details)

Rebase

This revision was landed with ongoing or failed builds.May 8 2023, 9:38 PM
This revision was automatically updated to reflect the committed changes.