Page MenuHomePhabricator

[PGO] Enable preinline and cleanup when optimize for size
ClosedPublic

Authored by zequanwu on Nov 17 2020, 6:03 PM.

Details

Summary

This is intended to reduce the binary size of both phase 1 and phase 2 builds.
By compiling chrome on Windows (using -Os in both phases), the binary size of phase 1 is reduced by 27.5% and binary size of phase 2 is reduced by 8.4%.

Diff Detail

Event Timeline

zequanwu created this revision.Nov 17 2020, 6:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2020, 6:03 PM
zequanwu requested review of this revision.Nov 17 2020, 6:03 PM
zequanwu updated this revision to Diff 305948.Nov 17 2020, 6:06 PM

update wrong diff.

Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2020, 6:06 PM
xur added a comment.Nov 17 2020, 8:21 PM

This is probably OK for -Os (SizeLevel == 1), but we need to be careful with Oz (SizeLevel == 2).
We already know that enabling preinliner in general will reduce size -- as the preinliner is pretty conservative. But there will be cases size will be increased.

I would like more test results (like bootstrap clang) before committing.

rnk added a comment.Nov 18 2020, 11:28 AM

Zequan, to build clang with PGO, you can follow the steps in Chrome's script to build clang with PGO:
https://source.chromium.org/chromium/chromium/src/+/master:tools/clang/scripts/build.py;l=703?q=clang%2Fscripts%2F%20build.py&ss=chromium

Regarding -Oz / minsize / SizeLevel 2, what we have discovered is that *not* running the preinliner makes the instrumented binary almost unusably large. Even if 75 is the wrong inline threshold for minsize, it's better than not running the preinliner. I think we should run with it.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
334

This code is duplicated for the NPM and the old PM. Please keep them in sync.

zequanwu added a comment.EditedNov 30 2020, 2:27 PM
In D91673#2401578, @xur wrote:

This is probably OK for -Os (SizeLevel == 1), but we need to be careful with Oz (SizeLevel == 2).
We already know that enabling preinliner in general will reduce size -- as the preinliner is pretty conservative. But there will be cases size will be increased.

I would like more test results (like bootstrap clang) before committing.

I don't see binary size difference (<1%), when bootstrap clang with this change. I was using steps at https://llvm.org/docs/HowToBuildWithPGO.html#building-clang-with-pgo.

zequanwu updated this revision to Diff 308810.Dec 1 2020, 4:18 PM
  • sync NPM with LPM.
  • use PreInlineThreshold flag value instead of hardcode number.
rnk accepted this revision.Dec 1 2020, 4:43 PM

Thanks, my concerns are addressed, and I believe @xur's are as well. Please wait for his input before landing, though.

This revision is now accepted and ready to land.Dec 1 2020, 4:43 PM
xur accepted this revision.Dec 8 2020, 4:57 PM

Sorry to the late response (I somehow missed the reply).

(1) Can we add some comments to explain the reason for enabling this for -Oz -- I think the comments from @rnk is useful to some people (at least to me).

(2) Also please add a testcase.

Feel free to commit to address the above two issues.

rnk added a comment.Dec 9 2020, 11:04 AM

I see two pre-commit test failures on the patch. Can you take a look at those as well?

zequanwu updated this revision to Diff 310619.Dec 9 2020, 12:45 PM
  • fix existing failed testcase.
  • add new testcases for npm and lpm.
  • add comments.
This revision was landed with ongoing or failed builds.Dec 10 2020, 12:29 PM
This revision was automatically updated to reflect the committed changes.