Page MenuHomePhabricator

[LowerTypeTests] Limit when icall jumptable entries are emitted
ClosedPublic

Authored by vlad.tsyrklevich on Jun 1 2018, 1:56 PM.

Details

Summary

Currently LowerTypeTests emits jumptable entries for all live external
and address-taken functions; however, we could limit the number of
functions that we emit entries for significantly.

For Cross-DSO CFI, we continue to emit jumptable entries for all
exported definitions. In the non-Cross-DSO CFI case, we only need to
emit jumptable entries for live functions that are address-taken in live
functions. This ignores exported functions and functions that are only
address taken in dead functions. This change uses ThinLTO summary data
(now emitted for all modules during ThinLTO builds) to determine
address-taken and liveness info.

The logic for emitting jumptable entries is more conservative in the
regular LTO case because we don't have summary data in the case of
monolithic LTO builds; however, once summaries are emitted for all LTO
builds we can unify the Thin/monolithic LTO logic to only use summaries
to determine the liveness of address taking functions.

This change is a partial fix for PR37474. It reduces the build size for
nacl_helper by ~2-3%, the reduction is due to nacl_helper compiling in
lots of unused code and unused functions that are address taken in dead
functions no longer being being considered live due to emitted jumptable
references. The reduction for chromium is ~0.1-0.2%.

Diff Detail

Repository
rL LLVM

Event Timeline

vlad.tsyrklevich retitled this revision from [LowerTypeTests] Limit when which jumptable entries are emitted to [LowerTypeTests] Limit when icall jumptable entries are emitted.Jun 1 2018, 1:57 PM
pcc added inline comments.Jul 3 2018, 1:53 PM
lib/Transforms/IPO/LowerTypeTests.cpp
1715 ↗(On Diff #149539)

Wouldn't this apply not just to ExternalLinkage but to other non-local linkages?

1779 ↗(On Diff #149539)

Use dyn_cast instead of isa to avoid needing the cast below.

1788 ↗(On Diff #149539)

Same comment about ExternalLinkage.

test/Transforms/LowerTypeTests/function-ext.ll
10 ↗(On Diff #149539)

With this change I don't think this test is testing what it is intended to test any more. Can you insert an artificial reference to foo?

  • Address Peter's comments
lib/Transforms/IPO/LowerTypeTests.cpp
1715 ↗(On Diff #149539)

You're right, I care that they're not exported, not that they're not external specifically.

test/Transforms/LowerTypeTests/function-ext.ll
10 ↗(On Diff #149539)

If we address take @foo, then WASM32 @bar won't return false immediately. I've added tests that exercise both the addr-taken and non-addr-taken cases since WASM and X64 have similar behavior now.

pcc accepted this revision.Jul 12 2018, 11:22 AM

LGTM

lib/Transforms/IPO/LowerTypeTests.cpp
1715 ↗(On Diff #149539)

I might write this a little more straightforwardly as

if (GVS->isLive() && !GlobalValue::isLocalLinkage(GVS->linkage()))
  Exported = true;
This revision is now accepted and ready to land.Jul 12 2018, 11:22 AM
This revision was automatically updated to reflect the committed changes.