Page MenuHomePhabricator

Cost Annotation Writer for InlineCost
Needs ReviewPublic

Authored by knaumov on Dec 13 2019, 6:11 PM.

Details

Summary

The patch adds extra diagnostics for the inline cost analysis. The diagnostic is available under the new command line flag ("print-cost-annotation-writer") for the "inline-cost" pass. When enabled along with -debug-only=inline-cost it prints the IR of inline candidate annotated with cost and threshold increase per every instruction.
This should help analyze inliner decisions.

Diff Detail

Event Timeline

knaumov created this revision.Dec 13 2019, 6:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2019, 6:12 PM
apilipenko added inline comments.Dec 16 2019, 10:31 AM
llvm/lib/Analysis/InlineCost.cpp
1847–1848

Code style, { should be on the same line. Please clang-format.

knaumov updated this revision to Diff 234193.Dec 16 2019, 4:59 PM
knaumov added reviewers: efriedma, davidxl.

Refactoring

knaumov marked an inline comment as done.Jan 7 2020, 9:05 AM

ping

This looks useful. Is it possible to add a test case?

davidxl added a reviewer: kazu.Jan 7 2020, 9:24 AM
knaumov updated this revision to Diff 238574.Jan 16 2020, 12:25 PM
knaumov added a reviewer: mtrofin.

Sorry for the delay due to the changes in structure of InlineCost.cpp in this patch: https://reviews.llvm.org/D71733 .
Structural changes mostly.
Added test to llvm/test/DebugInfo to test the new flag.

mtrofin added inline comments.Jan 16 2020, 12:42 PM
llvm/lib/Analysis/InlineCost.cpp
112

Nit: consider replacing std::pair<int,int> with a struct, e.g. struct InliningCostDetail {int Cost, int Threshold} - for readability.

585

I initially thought that the map stores a snapshot of Cost & Threshold values. It seems it actually stores cost and threshold variations due to the analysis of a specific instruction - relative cost of that one instruction, and threshold effects.

If that's correct, could you please detail this in the comment describing the map (or struct members, if using a struct instead of std::pair)

knaumov updated this revision to Diff 238899.Jan 17 2020, 2:59 PM

Changed pait<int, int> to struct InlineCostDetails in DenseMap and changed comment defining CostThrehsoldMap.
@mtrofin : Yes, the DenseMap stores the delta of the cost and threshold after inlining the instruction, not just a snapshot.
This is done in order to see the exact cost/change in the threshold it would take to inline the instruction.

knaumov marked 2 inline comments as done.Jan 17 2020, 3:01 PM

lgtm

llvm/lib/Analysis/InlineCost.cpp
108

Nit: CostDelta / ThresholdDelta? (I realize I proposed 'Cost/Threshold' initially, but that was before understanding the usage further below)

Also nit: now that we have a struct, if we had CostBefore/CostAfter and ThresholdBefore/ThresholdAfter, the intent would be even more evident, and we could, if we wanted, also output the raw costs (i.e. costs and delta). But this could be done at a later stage, too.

knaumov updated this revision to Diff 239366.Tue, Jan 21, 10:08 AM

Answering @mtrofin comments.
Refactoring, little changes:

  • Separated the initial flag into two: one prints the whole info on instruction cost and threshold, the other one prints only deltas of cost and threshold
  • Changed the InlineCostDetail structure - introduced CostBefore, CostAfter, CostDelta, ThresholdBefore, ThresholdAfter, ThresholdDelta
  • Changed the test that I wrote to match the check of new flags
  • Little comment changes
knaumov marked an inline comment as done.Mon, Jan 27, 11:19 AM

ping

RKSimon added inline comments.
llvm/test/DebugInfo/debuginline-cost-delta.ll
118

Is all this necessary?

knaumov updated this revision to Diff 240972.Tue, Jan 28, 12:31 PM

Changed unnecessarily complicated test

apilipenko added inline comments.Fri, Jan 31, 2:46 PM
llvm/lib/Analysis/InlineCost.cpp
110–115

You don't have to store deltas in the struct. They are easily computable from other fields.

knaumov updated this revision to Diff 242124.Mon, Feb 3, 10:36 AM
knaumov marked 2 inline comments as done.

Resolving @apilipenko suggestions.
Removed deltas from the struct.