This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Switch to using layout order in YAML
ClosedPublic

Authored by Amir on Jul 17 2023, 2:17 PM.

Details

Reviewers
spupyrev
rafauler
maksfb
Group Reviewers
Restricted Project
Commits
rG69b7e257fec6: [BOLT] Switch to using layout order in YAML
Summary

Use layout order in YAML profile reading/writing. Preserve old behavior (DFS order)
under -profile-use-dfs option.

Diff Detail

Event Timeline

Amir created this revision.Jul 17 2023, 2:17 PM
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Amir requested review of this revision.Jul 17 2023, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 2:17 PM
spupyrev added inline comments.Jul 18 2023, 9:22 AM
bolt/lib/Profile/YAMLProfileReader.cpp
109

Is there a reason why the type is not BinaryFunction::BasicBlockOrderType but an explicit SmallVector?

Also I'm not sure if there is any advantage here to use SmallVector over std::vector, as the order is always non-empty.

Amir updated this revision to Diff 541648.Jul 18 2023, 11:13 AM
Amir edited the summary of this revision. (Show Details)

Use BasicBlockOrderType

Amir marked an inline comment as done.Jul 18 2023, 11:15 AM
Amir added inline comments.
bolt/lib/Profile/YAMLProfileReader.cpp
109

You're right, can just use that type alias. I thought to use a SmallVector<const BinaryBasicBlock *, 0> version in YAMLWriter but let's keep them uniform between reader and writer. Regarding std::vector: it used to be it, but Maksim changed to ADT type citing BinaryFunction size reduction.

spupyrev accepted this revision.Jul 18 2023, 12:35 PM
This revision is now accepted and ready to land.Jul 18 2023, 12:35 PM
maksfb requested changes to this revision.Jul 18 2023, 2:29 PM

Since you are changing the default behavior, retitle the diff to reflect that fact. Otherwise LGTM.

This revision now requires changes to proceed.Jul 18 2023, 2:29 PM
Amir retitled this revision from [BOLT] Introduce ProfileUseDFS option to [BOLT] Switch to using layout order in YAML.Jul 18 2023, 2:31 PM
Amir edited the summary of this revision. (Show Details)

"Use layout order for YAML profile"?

This revision was not accepted when it landed; it landed in state Needs Revision.Jul 18 2023, 2:34 PM
This revision was automatically updated to reflect the committed changes.
Amir marked an inline comment as done.

It's fine :)