This is an archive of the discontinued LLVM Phabricator instance.

[WPD] Fix handling of pure virtual base class
ClosedPublic

Authored by tejohnson on Feb 20 2021, 2:20 PM.

Details

Summary

The fix in 3c4c205060c9398da705eb71b63ddd8a04999de9 caused an assert in
the case of a pure virtual base class. In that case, the vTableFuncs
list on the summary will be empty, so we were hitting the new assert
that the linkage type was not available_externally.

In the case of pure virtual, we do not want to assert, and additionally
need to set VS so that we don't treat it conservatively and quit the
analysis of the type id early.

This exposed a pre-existing issue where we were not updating the vcall
visibility on pure virtual functions when whole program visibility was
specified. We were skipping updating the visibility on any global vars
that didn't have any vTableFuncs, which meant all pure virtual were not
updated, and the later analysis would block any devirtualization of
calls that had a type id used on those pure virtual vtables (see the
handling in the other code modified in this patch). Simply remove that
check. It will mean that we may update the vcall visibility on global
vars that aren't vtables, but that setting is ignored for any global
vars that didn't have type metadata anyway.

Added a new test case that asserted without removing the assert, and
that requires the other fixes in this patch (updateVCallVisibilityInIndex
and not skipping all vtables without virtual funcs) to get a successful
devirtualization with index-only WPD. I added cases to test hybrid and
regular LTO for completeness, although those already worked without the
fixes here.

With this final fix, a clang multistage bootstrap with WPD builds and
runs all tests successfully.

Diff Detail

Event Timeline

tejohnson created this revision.Feb 20 2021, 2:20 PM
tejohnson requested review of this revision.Feb 20 2021, 2:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2021, 2:20 PM
MaskRay added inline comments.Feb 20 2021, 4:00 PM
llvm/test/ThinLTO/X86/devirt_pure_virtual_base.ll
7

Seems that this needs to be a ld.lld test and provides a definition for __cxa_pure_virtual.

42

%t1b.o.4.opt.bc does not exist.

tejohnson added inline comments.Feb 20 2021, 9:40 PM
llvm/test/ThinLTO/X86/devirt_pure_virtual_base.ll
7

Oops, forgot to remove these. Note they aren't actually running since I removed the : after RUN.

42

Good catch. I had this file sitting around from an earlier version. Fixed.

MaskRay accepted this revision.Feb 23 2021, 10:51 AM

Added a new test case that asserted without removing the assert, and that requires the other fixes in this patch to get a successful devirtualization with index-only WPD.

Nit: other -> updateVCallVisibilityInIndex just to make it clearer:)

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
1030

Ok, I understand the "However" part now. Question: does the "Previously" part imply that mixing old bitcode could be a problem?

llvm/test/ThinLTO/X86/devirt_pure_virtual_base.ll
89

; CHECK-IR-NEXT: } assuming you want to test that there is exactly one ret.

This revision is now accepted and ready to land.Feb 23 2021, 10:51 AM
tejohnson marked an inline comment as done.Feb 23 2021, 11:10 AM

Added a new test case that asserted without removing the assert, and that requires the other fixes in this patch to get a successful devirtualization with index-only WPD.

Nit: other -> updateVCallVisibilityInIndex just to make it clearer:)

There are 2 other fixes, one is updateVCallVisibilityInIndex and one is the change not to skip vtables without virtual functions. I'll update the summary to clarify.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
1030

Not specifically mixing of old and new, just using old bitcode with a newer linker. Also old tests, but those could be fixed, more concerned about trying to link together bitcode produced by an earlier clang.

Update test per comment

tejohnson edited the summary of this revision. (Show Details)Feb 23 2021, 11:10 AM
MaskRay accepted this revision.Feb 23 2021, 11:27 AM
This revision was automatically updated to reflect the committed changes.