This is an archive of the discontinued LLVM Phabricator instance.

Change the cap on the amount of padding for each vtable to 32-byte (previously it was 128-byte)
ClosedPublic

Authored by zhaomo on Jul 16 2018, 3:44 PM.

Details

Summary

We tested different cap values with a recent commit of Chromium. Our results show that the 32-byte cap yields the smallest binary and all the caps yield similar performance.
Based on the results, we propose to change the cap value to 32-byte.

Event Timeline

zhaomo created this revision.Jul 16 2018, 3:44 PM
zhaomo updated this revision to Diff 155782.Jul 16 2018, 4:05 PM
  • removed .arcconfig
zhaomo edited the summary of this revision. (Show Details)Jul 16 2018, 4:13 PM
zhaomo edited the summary of this revision. (Show Details)Jul 16 2018, 5:07 PM
zhaomo added reviewers: vlad.tsyrklevich, pcc.
dberris added inline comments.
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
774–778

Is it possible to have a reference to the experiment results either through a link or a document or in a comment?

zhaomo planned changes to this revision.Jul 18 2018, 2:35 PM
zhaomo added inline comments.
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
774–778

How about a link to the email of the experiment results (http://lists.llvm.org/pipermail/llvm-dev/2018-July/124694.html)? Is the llvm-dev archive considered permanent?

dberris added inline comments.Jul 18 2018, 4:26 PM
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
774–778

If it could be summarised somewhere in this file, it would be more permanent and less cryptic than the current comment. I can suggest a summary of the methodology, for example, even citing the trade-off being done. Something like:

Experiments with some large applications with a non-trivial number of dynamic types
have shown a binary size reduction of N% with virtually no measurable runtime overhead.
This padding used to be larger (128) and based on more recent experiments (see
<link to email or something in the LLVM docs/ directory>) we found that it could be
reduced to 32 and get significant binary size reductions with comparable runtime
performance.

Alternatively, if we can make the number a function of the cache line size (say, instead of 32 use the target's cache line size and derive something like 32 from it) which might/could explain why aligning to these numbers on those boundaries would not cause significant slowdowns but still have good binary size effects/properties. If I read the experiment results I can see that the platforms being built for/tested are 64-bit platforms with (I think) 64-byte cache lines.

zhaomo updated this revision to Diff 156185.Jul 18 2018, 5:04 PM

Added more information about the experiments in the comment.

pcc added a comment.Jul 18 2018, 5:28 PM

This looks fine if Dean is happy.

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

Remove the word "now".

dberris accepted this revision.Jul 18 2018, 7:24 PM

LGTM -- +1 to @pcc's suggestion on wording of the comment.

This revision is now accepted and ready to land.Jul 18 2018, 7:24 PM
zhaomo updated this revision to Diff 156339.Jul 19 2018, 12:38 PM

Updated the comment according to Peter's suggestion.

pcc added a comment.Jul 19 2018, 1:35 PM

It looks like this patch causes the check-cfi tests to fail, can you please take a look?

zhaomo updated this revision to Diff 156581.Jul 20 2018, 1:21 PM

Some tests failed because they did not expect the new layouts caused by the change to the padding cap. I modified those tests so that now they accept the new layouts.

This revision was automatically updated to reflect the committed changes.