This is an archive of the discontinued LLVM Phabricator instance.

[Inliner] Preserve !prof metadata when converting call to invoke.
ClosedPublic

Authored by hoy on May 9 2022, 11:24 AM.

Details

Summary

When a callee function is inlined via an invoke instruction, every function call inside the callee, if not an invoke, will be converted to an invoke after cloned to the caller body. I found that during the conversion the !prof metadata was dropped. This in turned caused a cloned indirect call not properly promoted in subsequent passes.

The particular scenario I was investigating was with AutoFDO and thinLTO. In prelink, no ICP was triggered (neither by the sample loader nor PGO ICP), no indirect call was promoted. This is because 1) the particular indirect call did not have inlined samples; and 2) PGO ICP was intentionally disabled. After inlining, the prof metadata was dropped. Then in postlink, PGO ICP jumped in but didn't do anything. Thus the opportunity was missed.

I'm making a simple fix to preserve !prof metadata when converting call to invoke.

Diff Detail

Event Timeline

hoy created this revision.May 9 2022, 11:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 11:24 AM
hoy requested review of this revision.May 9 2022, 11:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 11:24 AM
hoy edited the summary of this revision. (Show Details)May 9 2022, 11:37 AM
hoy added reviewers: wenlei, wlei, davidxl.

Does the data need to be scaled first?

Does the data need to be scaled first?

Scaling is done right after inlining, separately. Here when just converting call to invoke, we can preserved the counts faithfully.

Should we have a test case to cover the post-inlining scaling. Otherwise looks good.

hoy added a comment.May 9 2022, 12:05 PM

Does the data need to be scaled first?

Good point. The scaling should be done earlier. Similarly for !dbg metadata.

hoy added a comment.May 9 2022, 12:05 PM

Should we have a test case to cover the post-inlining scaling. Otherwise looks good.

Will do.

hoy updated this revision to Diff 428159.May 9 2022, 12:22 PM

Extending test to check scaling.

davidxl accepted this revision.May 9 2022, 12:53 PM

lgtm

This revision is now accepted and ready to land.May 9 2022, 12:53 PM
This revision was landed with ongoing or failed builds.May 9 2022, 3:08 PM
This revision was automatically updated to reflect the committed changes.