This is an archive of the discontinued LLVM Phabricator instance.

[WPD/VFE] Always emit vcall_visibility metadata for -fwhole-program-vtables
ClosedPublic

Authored by tejohnson on Dec 26 2019, 8:47 AM.

Details

Summary

First patch to support Safe Whole Program Devirtualization Enablement,
see RFC here: http://lists.llvm.org/pipermail/llvm-dev/2019-December/137543.html

Always emit !vcall_visibility metadata under -fwhole-program-vtables,
and not just for -fvirtual-function-elimination. The vcall visibility
metadata will (in a subsequent patch) be used to communicate to WPD
which vtables are safe to devirtualize, and we will optionally convert
the metadata to hidden visibility at link time. Subsequent follow on
patches will help enable this by adding vcall_visibility metadata to the
ThinLTO summaries, and always emit type test intrinsics under
-fwhole-program-vtables (and not just for vtables with hidden
visibility).

In order to do this safely with VFE, since for VFE all vtable loads must
be type checked loads which will no longer be the case, this patch adds
a new "Virtual Function Elim" module flag to communicate to GlobalDCE
whether to perform VFE using the vcall_visibility metadata.

One additional advantage of using the vcall_visibility metadata to drive
more WPD at LTO link time is that we can use the same mechanism to
enable more aggressive VFE at LTO link time as well. The link time
option proposed in the RFC will convert vcall_visibility metadata to
hidden (aka linkage unit visibility), which combined with
-fvirtual-function-elimination will allow it to be done more
aggressively at LTO link time under the same conditions.

Diff Detail

Event Timeline

tejohnson created this revision.Dec 26 2019, 8:47 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 26 2019, 8:47 AM
tejohnson marked an inline comment as done.Dec 26 2019, 9:10 AM
tejohnson added inline comments.
llvm/lib/IR/Metadata.cpp
1506

The erasing of old metadata is needed to enable upgrading the visibility to hidden (linkage unit) in a subsequent patch. Made that change here as well as the associated rename of the method to try to contain the main vcall visibility metadata changes in a single patch.

Looks good at first sight. Do you have linker patch for me to try this out?

Looks good at first sight. Do you have linker patch for me to try this out?

The linker changes are in D71913. This one should be a noop by itself (other than just getting the additional metadata). You need all three (this one, D71911 and D71913) to implement the RFC.

evgeny777 added inline comments.Jan 21 2020, 8:02 AM
clang/lib/CodeGen/ItaniumCXXABI.cpp
676 ↗(On Diff #238561)

Why are we checking for hidden visibility here?

tejohnson marked an inline comment as done.Jan 21 2020, 12:32 PM
tejohnson added inline comments.
clang/lib/CodeGen/ItaniumCXXABI.cpp
676 ↗(On Diff #238561)

This one is leftover from an earlier attempt at doing something more clever during GlobalOpt to decide whether we should be doing VFE or not, and I eventually abandoned that as it didn't work well, and went with the module flag here. You're right that we should be emitting the type test here under WholeProgramVtables, and not just with hidden visibility, but that belongs in D71913 (where I make the other change to insert type tests without hidden visibility). I've removed this change from this patch and am going to add it along with a test to that later patch.

Remove stale change that didn't belong here.

evgeny777 added inline comments.Jan 23 2020, 1:46 AM
llvm/lib/Transforms/IPO/GlobalSplit.cpp
116

I think this needs a test. Removal of this code doesn't break anything

tejohnson marked 2 inline comments as done.Jan 23 2020, 6:56 AM
tejohnson added inline comments.
llvm/lib/Transforms/IPO/GlobalSplit.cpp
116

Adding some checking in a GlobalSplit test.

tejohnson updated this revision to Diff 239888.Jan 23 2020, 6:57 AM
tejohnson marked an inline comment as done.

Test GlobalSplit handling of vcall_visibility metadata

This revision is now accepted and ready to land.Jan 23 2020, 7:11 AM
This revision was automatically updated to reflect the committed changes.