Page MenuHomePhabricator

[LTO/WPD] Enable aggressive WPD under LTO option
ClosedPublic

Authored by tejohnson on Dec 26 2019, 11:53 AM.

Details

Summary

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

This patch adds type test metadata under -fwhole-program-vtables,
even for classes without hidden visibility. It then changes WPD to skip
devirtualization for a virtual function call when any of the compatible
vtables has public vcall visibility.

Additionally, internal LLVM options as well as lld and gold-plugin
options are added which enable upgrading all public vcall visibility
to linkage unit (hidden) visibility during LTO. This enables the more
aggressive WPD to kick in based on LTO time knowledge of the visibility
guarantees.

Support was added to all flavors of LTO WPD (regular, hybrid and
index-only), and to both the new and old LTO APIs.

Unfortunately it was not simple to split the first and second parts of
this part of the change (the unconditional emission of type tests and
the upgrading of the vcall visiblity) as I needed a way to upgrade the
public visibility on legacy WPD llvm assembly tests that don't include
linkage unit vcall visibility specifiers, to avoid a lot of test churn.

I also added a mechanism to LowerTypeTests that allows dropping type
test assume sequences we now aggressively insert when we invoke
distributed ThinLTO backends with null indexes, which is used in testing
mode, and which doesn't invoke the normal ThinLTO backend pipeline.

Depends on D71907 and D71911.

Diff Detail

Event Timeline

tejohnson created this revision.Dec 26 2019, 11:53 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
evgeny777 added inline comments.Jan 13 2020, 7:09 AM
clang/lib/CodeGen/BackendUtil.cpp
563

Test case?

clang/test/CodeGenCXX/lto-visibility-inference.cpp
73

What caused this and other changes in this file?

llvm/include/llvm/Transforms/IPO.h
247

s/StripAll/DropTypeTests/g ?

llvm/lib/LTO/LTOCodeGenerator.cpp
550

I'd rather use

updateVCallVisibilityInModule(*MergedModule,  /* WholeProgramVisibilityEnabledInLTO */ false)

and remove default value for second parameter

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
975

ditto

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1775

Fold identical code blocks

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

Is this tested?

ormris added a subscriber: ormris.Jan 13 2020, 11:39 AM
tejohnson marked 9 inline comments as done.Jan 14 2020, 11:06 AM
tejohnson added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
563

See clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp. Specifically the second block of clang invocations:

// Also check type test are lowered when the distributed ThinLTO backend clang
// invocation is passed an empty index file, in which case a non-ThinLTO
// compilation pipeline is invoked. If not lowered then LLVM CodeGen may assert.

I'm testing both the new and old PMs, as well as the new PM O0 path (basically all paths modified in this file).

clang/test/CodeGenCXX/lto-visibility-inference.cpp
73

Because we now will insert type tests for non-hidden vtables. This is enabled by the changes to LTO to interpret these based on the vcall_visibility metadata.

llvm/include/llvm/Transforms/IPO.h
247

Woops, missed this one after my rename! Good catch.

Also, noticed the description in the comments is now stale, fixed.

llvm/lib/LTO/LTOCodeGenerator.cpp
550

Good idea, changed.

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1775

After looking at this code again, I have changed it a bit. We should only be removing assumes that are consuming type test instructions. I have changed this (which removes the redundancy in any case). I also added a check to the test mentioned earlier for this handling (clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp) to ensure we don't remove unrelated llvm.assume.

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

Added a test of it to llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll.

tejohnson marked 2 inline comments as done.

Address comments

Thanks, I'm still in process of testing (now fixing issue which however is most likely related to devirtualization itself, not to this patch). Meanwhile some of my comments below.

llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
2 ↗(On Diff #238048)

Why do you need -whole-program-visibility here? Correct me if I'm wrong, but AFAIK module scanning doesn't happen during import and GV visibility should be taken from imported summary.

llvm/tools/opt/opt.cpp
633

Hm, looks like I don't fully understand this. I have following concerns:

  1. According to your changes every time I use opt -wholeprogramdevirt I also have to pass -whole-program-visibility. Has -wholeprogramdevirt flag become no-op without this additional flag? If so this looks counter intuitive to me.
  1. When I use opt -wholeprogramdevirt default constructor of WholeProgramDevirt class is called and UseCommandLine flag is set to true. Can't I use this flag to effectively lower visibility in module instead of playing with metadata?
if (VS->vCallVisibility() == GlobalObject::VCallVisibilityPublic && !UseCommandLine)
     return false;
tejohnson marked 2 inline comments as done.Jan 16 2020, 9:16 AM
tejohnson added inline comments.
llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
2 ↗(On Diff #238048)

Before my patch, type tests were only inserted for vtables with hidden LTO visibility. Therefore, the very presence of type tests communicated the hidden visibility and enabled the WPD.

With this patch, to support enabling WPD aggressively at LTO time, we now insert type tests unconditionally under -fwhole-program-vtables. The vcall_visibility metadata tells LTO how to interpret them. And the new options allow changing those to hidden visibility to get the more aggressive WPD.

Because these legacy tests have type tests but no vcall_visibility metadata, we now will conservatively treat them as having public LTO visibility. This option is therefore required to convert the summarized (default public) vcall visibility into hidden to get the prior more aggressive behavior they are trying to test.

Note I could have instead changed the assembly here to add hidden vcall_visibility metadata everywhere. That seemed a little onerous so that's why I just added the option. I could add a comment to that effect if it would help?

llvm/tools/opt/opt.cpp
633

According to your changes every time I use opt -wholeprogramdevirt I also have to pass -whole-program-visibility. Has -wholeprogramdevirt flag become no-op without this additional flag? If so this looks counter intuitive to me.

No, it wouldn't be needed if the tests contained !vcall_visibility metadata indicating hidden LTO visibility (see my earlier comment response).

When I use opt -wholeprogramdevirt default constructor of WholeProgramDevirt class is called and UseCommandLine flag is set to true. Can't I use this flag to effectively lower visibility in module instead of playing with metadata?

I could do that. What it would mean though is that we would be unable to use opt for any future testing of vtables intended to have public vcall visibility (either through a lack of that metadata, or explicit vcall_vsibility metadata indicating public). Which might be ok - in fact all my new testing of this behavior is via llvm-lto2 or the linkers. I suppose that would obviate this change as well as all the opt based test changes to pass the flag. Do you think that is better?

evgeny777 added inline comments.Jan 17 2020, 11:10 AM
llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
2 ↗(On Diff #238048)

I think you can remove this option from this test (and probably others using -wholeprogramdevirt-summary-action=import option), because visibility seems to be not analyzed on import phase. I just did this btw and test still passes flawlessly.

llvm/tools/opt/opt.cpp
633

I could do that. What it would mean though is that we would be unable to use opt for any future testing of vtables intended to have public vcall visibility (either through a lack of that metadata, or explicit vcall_vsibility metadata indicating public). Which might be ok - in fact all my new testing of this behavior is via llvm-lto2 or the linkers. I suppose that would obviate this change as well as all the opt based test changes to pass the flag. Do you think that is better?

Thanks for explanation. I think it's okay to have this extra option for devirtualization to work with legacy IR files using opt. But please add comment somewhere documenting why exactly this option is needed (probably near its definition in WholeProgramDevirt.cpp)

tejohnson marked 4 inline comments as done.Jan 20 2020, 4:55 PM
tejohnson added inline comments.
llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
2 ↗(On Diff #238048)

Good point, removed from here and a couple other similar tests. I added it a little overeagerly to address the failures I got originally.

llvm/tools/opt/opt.cpp
633

Added a comment documenting the need to where the option is defined.

tejohnson updated this revision to Diff 239214.Jan 20 2020, 4:56 PM
tejohnson marked 2 inline comments as done.

Address comments

evgeny777 added inline comments.Jan 21 2020, 6:57 AM
clang/test/CodeGenCXX/lto-visibility-inference.cpp
73

The results of this test case

%clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 -fms-extensions -fwhole-program-vtables -flto-visibility-public-std -emit-llvm -o - %s | FileCheck --check-prefix=MS --check-prefix=MS-NOSTD %s

look not correct to me. I think you shouldn't generate type tests for standard library classes with -flto-visibility-public-std. Currently if this flag is given, clang doesn't do this either even with -fvisibility=hidden

clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
8

I think, we no longer need -fvisibility hidden here, do we?

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
975

updateVCallVisibilityInIndex(*Index, /* WholeProgramVisibilityEnabledInLTO */ false) ?

tejohnson marked 5 inline comments as done.Jan 21 2020, 1:53 PM
tejohnson added inline comments.
clang/test/CodeGenCXX/lto-visibility-inference.cpp
73

The associated vtables would get the vcall_visibility public metadata, so the type tests themselves aren't problematic. I tend to think that an application using such options should simply stick with -fvisibility=hidden to get WPD and not use the new LTO option to convert all public vcall_visibility metadata to hidden.

clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
8

Right, we aren't relying on the visibility for this test which is just ensuring the type tests are lowered properly in the distributed backends. I removed it.

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
975

Missed this one before (did the InModule version but not InIndex), fixed.

tejohnson updated this revision to Diff 239428.Jan 21 2020, 1:56 PM
tejohnson marked 2 inline comments as done.

Address comments and rebase. Also apply modified change to ItaniumCXXABI.cpp
and a change to an associated test (cfi-mfcall.cpp) here as discussed
in child revision D71907.

evgeny777 added inline comments.Jan 22 2020, 4:55 AM
clang/test/CodeGenCXX/lto-visibility-inference.cpp
73

The associated vtables would get the vcall_visibility public metadata, so the type tests themselves aren't problematic. I tend to think that an application using such options should simply stick with -fvisibility=hidden to get WPD and not use the new LTO option to convert all public vcall_visibility metadata to hidden.

I see two issues here:

  1. It's not always good option to force hidden visibility for everything. For instance I work on proprietary platform which demands public visibility for certain symbols in order for dynamic loader to work properly. In this context your patch does a great job.
  1. Standard library is almost never LTOed so in general we can't narrow std::* vtables visibility to linkage unit

Is there anything which prevents from implementing the same functionality with new -lto-whole-program-visibility option (i.e without forcing hidden visibility)? In other words the following looks good to me:

# Compile with lto/devirtualization support
clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables -c *.cpp

# Link: everything is devirtualized except standard library classes virtual methods
clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
tejohnson marked an inline comment as done.Jan 22 2020, 9:03 AM
tejohnson added inline comments.
clang/test/CodeGenCXX/lto-visibility-inference.cpp
73

Ok, thanks for the info. I will go ahead and change the code to not insert the type tests in this case to support this usage.

Ultimately, I would like to decouple the existence of the type tests from visibility implications. I'm working on another change to delay lowering/removal of type tests until after indirect call promotion, so we can use them in other cases (streamlined indirect call promotion checks against the vtable instead of the function pointers, also useful if we want to implement speculative devirtualization based on WPD info). In those cases we need the type tests, either to locate information in the summary, or to get the address point offset for a vtable address compare. In that case it would be helpful to have the type tests in this type of code as well. So we'll need another way to communicate down to WPD that they should never be devirtualized. But I don't think it makes sense to design this support until there is a concrete use case and need. In the meantime I will change the code to be conservative and not insert the type tests in this case.

Address remaining comment by blocking type test insertion for public std visibility

Looks good so far. See remaining comment in D71907

clang/lib/CodeGen/CGVTables.cpp
1049–1050

nit: return !HasLTOVisibilityPublicStd(RD)

tejohnson marked an inline comment as done.Jan 23 2020, 7:00 AM
tejohnson updated this revision to Diff 239890.Jan 23 2020, 7:00 AM

Implement suggestion

evgeny777 accepted this revision.Jan 23 2020, 7:10 AM

LGTM, thanks!

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

FYI I reverted this in rG90e630a95ecc due to a cfi test failure in a windows sanitizer bot. Not sure what is happening, I'll need to try to debug it somehow tomorrow.

FYI I reverted this in rG90e630a95ecc due to a cfi test failure in a windows sanitizer bot. Not sure what is happening, I'll need to try to debug it somehow tomorrow.

This patch went back in a little while ago at 2f63d549f1e1edd165392837aaa53f569f7fb88d, and looks like it will stick this time. The windows bot no longer fails with this patch after the enabling fix in D73418 went in before this at
af954e441a5170a75687699d91d85e0692929d43.