This is an archive of the discontinued LLVM Phabricator instance.

[FuncSpec] Rework the discardment logic for unprofitable specializations.
ClosedPublic

Authored by labrinea on Aug 4 2023, 11:37 AM.

Details

Summary

Currently we make an arbitrary comparison between codesize and latency
in order to decide whether to keep a specialization or not. Sometimes
the latency savings are biased in favor of loops because of imprecise
block frequencies, therefore this metric contains a lot of noise. This
patch tries to address the problem as follows:

  • Reject specializations whose codesize savings are less than X% of the original function size.
  • Reject specializations whose latency savings are less than Y% of the original function size.
  • Reject specializations whose inlining bonus is less than Z% of the original function size.

I am not saying this is super precise, but at least X, Y and Z are
configurable, allowing us to tweak the cost model. Moreover, it lets
us prioritize codesize over latency, which is a less noisy metric.

I am also increasing the minimum size a function should have to be
considered a candidate for specialization. Initially the cost of
a function was calculated as

  
CodeMetrics::NumInsts * InlineConstants::getInstrCost()

which later in D150464 was altered into CodeMetrics::NumInsts since
the metric is supposed to model TargetTransformInfo::TCK_CodeSize.
However, we omitted adjusting MinFunctionSize in that commit.

Diff Detail

Event Timeline

labrinea created this revision.Aug 4 2023, 11:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 11:37 AM
labrinea requested review of this revision.Aug 4 2023, 11:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 11:37 AM
labrinea added inline comments.
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
902–903

I couldn't think of an example where this would be useful, let alone that calling getInlineCost is expensive according to this comment:
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Analysis/InlineCost.h#L274

ChuanqiXu added inline comments.Aug 6 2023, 6:47 PM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
864–865

I am wondering if the condition || may be too strict. My rough feeling is: it is good enough to perform specialization if we can see one of the benefits.

902–903

Maybe I just feel reusing the existing mature cost model may be a better choice. But given that is expensive and we're willing to build and fine-tunning our cost model. It may be good to not depend the expensieve getInlineCost.

933–938

Let's move the comment to the definition of the function. It will be much more clear.

labrinea added inline comments.Aug 7 2023, 1:10 AM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
864–865

Without the latency condition CTMark triggers too much. The two conditions seem to work quite well in conjunction, CTMark gets less specializations than before, which is good for compile times, but we still specialize the interesting cases (mcf, exchange).

ChuanqiXu accepted this revision.Aug 7 2023, 1:16 AM

LGTM with moving the comment.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
864–865

The result from benchmark is overwhelming : )

This revision is now accepted and ready to land.Aug 7 2023, 1:16 AM

@ChuanqiXu I was mistaken, the condition is indeed too strict. I'll rethink about it.

labrinea updated this revision to Diff 548278.Aug 8 2023, 10:41 AM
labrinea edited the summary of this revision. (Show Details)

Changes from last revision:

  • Increased the minimum function size
  • Added options to control the codesize/latency/inlining savings as percentages of the function size
labrinea requested review of this revision.Aug 8 2023, 10:42 AM

For the practical effects, I feel good as long as the interesting benchmarks remains optimized.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
108

What is a inlining saving? I am not sure if this is a wide known definition.

833

May this be a better name?

851

What's the meaning of the magic number 100?

851–857

nit: may this be slightly more clear?

labrinea added inline comments.Aug 8 2023, 11:58 PM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
108

I can rename it to MinInliningBonus

833

Ok

851

Percentage. We can't do floating point decision so we multiply by MinSavings and then divide by 100.

851

*division

851–857

Okay

labrinea added inline comments.Aug 9 2023, 12:37 AM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
833

Sorry I misunderstood. I am naming it score because at the end it contains InliningBonus + std::max(B.CodeSize, B.Latency).

labrinea updated this revision to Diff 548495.Aug 9 2023, 12:38 AM

Addressed review comments.

labrinea marked 7 inline comments as done.Aug 9 2023, 12:39 AM
ChuanqiXu accepted this revision.Aug 9 2023, 2:05 AM

LGTM then. Thanks.

This revision is now accepted and ready to land.Aug 9 2023, 2:05 AM
This revision was landed with ongoing or failed builds.Aug 9 2023, 2:33 AM
This revision was automatically updated to reflect the committed changes.