This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO/WPD] Handle function alias in vtable correctly
ClosedPublic

Authored by tejohnson on Feb 16 2023, 11:27 AM.

Details

Summary

We were not summarizing a function alias in the vtable, leading to
incorrect WPD in some cases, and missing WPD in others.

Specifically, we would end up ignoring function aliases as they aren't
summarized, so we could incorrectly devirtualize if there was a single
other non-alias function in a compatible vtable. And if there was only
one implementation, but it was an alias, we would not be able to
identify and perform the single implementation devirtualization.

Handling the alias summary correctly also required fixing the handling
in mustBeUnreachableFunction, so that it is not incorrectly ignored.

Regular LTO is conservatively correct because it will skip
devirtualizing when any pointer within a vtable is not a function.
However, it needs additional work to be able to take advantage of
function alias within the vtable that is in fact the only
implementation. For that reason, the Regular LTO testing in the second
test case is currently disabled, and will be enabled along with a follow
on enhancement fix for Regular LTO WPD.

Diff Detail

Event Timeline

tejohnson created this revision.Feb 16 2023, 11:27 AM
tejohnson requested review of this revision.Feb 16 2023, 11:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2023, 11:27 AM
mingmingl accepted this revision.Feb 16 2023, 12:46 PM
mingmingl added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
380–381
398–400

For my understanding, is this conservative change orthogonal with function aliases in vtable (since auto *FS = dyn_cast<FunctionSummary>(Summary->getBaseObject()) should be true for an alias)?

llvm/test/ThinLTO/X86/devirt_function_alias.ll
26
47
62
This revision is now accepted and ready to land.Feb 16 2023, 12:46 PM
xur accepted this revision.Feb 16 2023, 12:52 PM

LGTM.

tejohnson marked an inline comment as done.Feb 16 2023, 3:26 PM
tejohnson added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
398–400

It's related, but not strictly necessary for this case now that we get the base object of the summary a couple lines above. Without either of these fixes, we would incorrectly return true from this function for an alias.

llvm/test/ThinLTO/X86/devirt_function_alias.ll
26

It was on purpose because I was trying to match both "devirtualized" and "Devirtualized" (there are multiple messages on success, one is capitalized and the other isn't. I made this more explicit in the pattern.

47

see above

62

see above

tejohnson updated this revision to Diff 498170.Feb 16 2023, 3:26 PM

Address comments

This revision was automatically updated to reflect the committed changes.