This is an archive of the discontinued LLVM Phabricator instance.

Cost Annotation Writer for InlineCost
ClosedPublic

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
1846–1847

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
113

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

587

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
109

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.Jan 21 2020, 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.Jan 27 2020, 11:19 AM

ping

RKSimon added inline comments.
llvm/test/DebugInfo/debuginline-cost-delta.ll
117 ↗(On Diff #239366)

Is all this necessary?

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

Changed unnecessarily complicated test

apilipenko added inline comments.Jan 31 2020, 2:46 PM
llvm/lib/Analysis/InlineCost.cpp
111–116

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

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

Resolving @apilipenko suggestions.
Removed deltas from the struct.

apilipenko added inline comments.Feb 20 2020, 3:09 PM
llvm/lib/Analysis/InlineCost.cpp
51–57

Suggest to print all the details under one flag.

109

// This struct is used to store information about inline cost of a particular instruction

110

Suggest InstructionCostDetail.

121–124

No need to use llvm:: namespace prefix.

knaumov updated this revision to Diff 245768.Feb 20 2020, 4:55 PM
knaumov marked 4 inline comments as done.

Addressed @apilipenko 's comments

  • Switched to one flag
  • Small changes in struct and comments
apilipenko accepted this revision.Feb 21 2020, 1:53 PM

Couple of nit comments inlined, LGTM once addressed.

llvm/lib/Analysis/InlineCost.cpp
56

Suggest to rename the global and the flag to PrintDebugInstructionDeltas("print-instruction-deltas", ...)

124–125

There are still redundant llvm:: prefixes.

1838–1841

This comment is specific to InlineCostCallAnalyzer and doesn't belong here.

This revision is now accepted and ready to land.Feb 21 2020, 1:53 PM
knaumov updated this revision to Diff 246001.Feb 21 2020, 2:45 PM
knaumov marked 3 inline comments as done.

Fixed small inaccuracies
As the patch is done, @apilipenko , can you please push it for me?

knaumov updated this revision to Diff 246002.Feb 21 2020, 2:47 PM

Sorry, wrong diff

This revision was automatically updated to reflect the committed changes.