Page MenuHomePhabricator

Detect Source Drift with Propeller's basic-block-sections=list=
ClosedPublic

Authored by tmsriram on Jan 27 2021, 11:15 PM.

Details

Summary

Detect Source Drift with Propeller.

Source Drift happens when the sources are updated after profiling the binary but before building the final optimized binary. If the source has changed since the profiles were obtained, optimizing basic blocks might be sub-optimal. This only applies to BasicBlockSection::List as it creates clusters of basic blocks using basic block ids. Source drift can invalidate these groupings leading to sub-optimal code generation with regards to performance.

PGO source drift for a particular function can be detected using function metadata added in D95495.

Diff Detail

Event Timeline

tmsriram created this revision.Jan 27 2021, 11:15 PM
tmsriram requested review of this revision.Jan 27 2021, 11:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 11:15 PM
snehasish added inline comments.Jan 28 2021, 10:10 AM
llvm/lib/CodeGen/BasicBlockSections.cpp
92

Prefer naming the flag with a known prefix for easier identification. Eg. bbsections-detect-source-drift?

327

Perhaps hasInstrProfHashMismatch is a better name? I imagine in the future we will have more heuristics about detecting source drift where this is just one signal amongst many.

375

Should this be moved up before renumbering the blocks? I don't see a reason why we should renumber if we are going to bail out anyway.

llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll
1 ↗(On Diff #319769)

Can you rename this file to be consistent with the other basic-block-sections tests? Eg. basic-block-sections-source-drift.ll

2 ↗(On Diff #319769)

nit: Prefer hyphens to underscores to be consistent with the other basic block sections tests.

18–19 ↗(On Diff #319769)

This was quite confusing at first glance.
SOURCE-DRIFT-NOT is checking for the absence of .section .text (when the check is enabled)
NO_SOURCE_DRIFT is checking for the absence option being enabled.

Perhaps the second one is clearer if named HASH-CHECK-DISABLED?
Also is it worthwhile to add a test case for the no annotation case (common case) to catch future regression?

tmsriram updated this revision to Diff 320024.Jan 28 2021, 6:51 PM
tmsriram marked 6 inline comments as done.

Address all reviewer comments.

llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll
18–19 ↗(On Diff #319769)

There are already test cases with .ll files and without the metadata that check for bb sections. Is that alright?

snehasish accepted this revision.Jan 28 2021, 6:57 PM

lgtm

llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll
18–19 ↗(On Diff #319769)

Yes, sgtm.

This revision is now accepted and ready to land.Jan 28 2021, 6:57 PM
This revision was automatically updated to reflect the committed changes.