Page MenuHomePhabricator

[PGO] De-Optimizing cold functions based on PGO info (PATCH 1/2)
Needs ReviewPublic

Authored by myhsu on Tue, Sep 8, 5:07 PM.

Details

Summary

This patch disables optimizations on IR Functions that are considered “cold” by PGO
profiles. The primary goal for this work is to improve code optimization
speed (which also improves compilation and LTO speed) without making too
much impact on target code performance.

The mechanism is pretty simple: In the second phase (i.e. optimization
phase) of PGO, we would add optnone attributes on functions that are
considered “cold”. That is, functions with low profiling counts.

In addition to de-optimizing on functions whose profiling counts are
exactly zero (the -profile-omit-cold-func-opt flag). If the previous CLI flag
is enabled, another knob (-profile-omit-cold-func-opt-percent=<X percent>) can
be used to adjust the “cold threshold”. That is, after sorting profiling counts
of all functions, this knob provides an option to de-optimize functions
whose count values are sitting in the lower X percent.

Our evaluation results showed an average of 10% compilation speed
improvement with up to 2% performance overhead on various of thresholds using the
LLVM Test Suite as the benchmark. Using with FullLTO, it brought average
of 20% (LTO) link time improvement with up to 2% performance overhead.

Second part of this patch series: https://reviews.llvm.org/D87338

authors: myhsu, probinson, and edd

Diff Detail

Event Timeline

myhsu created this revision.Tue, Sep 8, 5:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 8, 5:07 PM
myhsu requested review of this revision.Tue, Sep 8, 5:07 PM

This is in theory similar to profile guided size optimization -- the difference is that optnone may not result in size reduction.

dmajor added a subscriber: dmajor.Tue, Sep 8, 5:27 PM

As per https://llvm.org/docs/LangRef.html, deopt term appears to be already coined.

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
1776

llvm::sort

As per https://llvm.org/docs/LangRef.html, deopt term appears to be already coined.

Yeah, I think deopt already means something else in the JIT compiler. I'd avoid the term.

uenoku added a subscriber: uenoku.Wed, Sep 9, 10:27 AM
myhsu added a comment.EditedWed, Sep 9, 11:20 AM

As per https://llvm.org/docs/LangRef.html, deopt term appears to be already coined.

Yeah, I think deopt already means something else in the JIT compiler. I'd avoid the term.

Good call, I didn't know that. I'll come up with an alternative one

myhsu updated this revision to Diff 290848.Wed, Sep 9, 5:09 PM
myhsu edited the summary of this revision. (Show Details)
  1. Fix minor formatting issues (NFC)
  2. Rename "profile-deopt-cold-*" into "profile-omit-cold-func-opt-*"
myhsu marked an inline comment as done.Wed, Sep 9, 5:09 PM

Implementation-wise, this looks good, but I wonder if we could join minsize with this using the same (or very similar) controls.

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
1808

Nit: keep the same language "Set optnone..."

llvm/test/Transforms/PGOProfile/omit-cold-func-opt.ll
12

IIUC, the range is inclusive. So, would be nice to have an additional 50% test to show that.

Implementation-wise, this looks good, but I wonder if we could join minsize with this using the same (or very similar) controls.

As I mentioned in the RFC thread, I think it's a good idea :-) Though I prefer to send out a separate review for the optsize / minsize work since I think they should have their own flags, given the fact that they're kinda conflict with optnone here (i.e. it doesn't really make sense to add both optsize and optnone at the same time).
Does that make sense to you?

myhsu updated this revision to Diff 291050.Thu, Sep 10, 12:08 PM
  • Fix wording (NFC)
  • Add 50% test and restructure the test file
myhsu marked 2 inline comments as done.Thu, Sep 10, 12:09 PM
myhsu added a comment.Mon, Sep 14, 4:11 PM

Ping. Just want to check out if we all agree to flush out this patch series first before supporting optsize / minsize as per our discussions in the RFC thread

Ping. Just want to check out if we all agree to flush out this patch series first before supporting optsize / minsize as per our discussions in the RFC thread

I'm happy as long as there's a short period between the two implementations, hopefully weeks. They should definitely not cross the release branch barrier.

But I haven't worked on the PGO side and I don't use PGO directly, so perhaps someone working on the area should voice their opinions, which will certainly be stronger than mine.

myhsu added a reviewer: vsk.Wed, Sep 16, 11:14 PM