[PATCH] D104751: Unpack the CostEstimate feature in ML inlining models.

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineModelFeatureMaps.h:104
+    "exposed externally")                                                      \
+  M(CostEstimate, "cost_estimate", "total cost estimate (threshold - free)")
 
----------------
why not leave this where it was? because you can now populate the remainder in a loop?


================
Comment at: llvm/include/llvm/Analysis/InlineModelFeatureMaps.h:117
 #undef POPULATE_INDICES
-      NumberOfFeatures
+
+  NumberOfFeatures
----------------
unrelated change?


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:441
 };
-
 /// FIXME: if it is necessary to derive from InlineCostCallAnalyzer, note
----------------
spurious change?


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:949
+
+  void update(InlineCostFeatureIndex Feature, int64_t Delta) {
+    Cost[static_cast<size_t>(Feature)] += Delta;
----------------
I think `increment` would be more clear. Then Delta can have default value 1, too (For the few cases that'd be the case)


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:982
+    update(InlineCostFeatureIndex::LoadRelativeIntrinsic,
+           3 * InlineConstants::InstrCost);
+  }
----------------
can this '3' be a const with some name (so it's easier to chase down any magic factors)


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1047
+      update(InlineCostFeatureIndex::CaseClusterPenalty,
+             NumCaseCluster * 2 * InlineConstants::InstrCost);
+      return;
----------------
can we replace this 2 and the 4 above with const values with a good name

same below - 3, 2 etc.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:947
+  int SingleBBBonus = 0;
+  int Threshold = 5;
+
----------------
jacobhegna wrote:
> mtrofin wrote:
> > Is this really a threshold, or can it have a name that better communicates what it accumulates? (apologies, can't right now figure what that may be)
> > 
> > same for the 2 bonuses above.
> these are copied from the heuristic inline visitor and used in almost the same way - there is some complexity in the other visitor that isn't replicated here. I'll keep the names the same to reflect that duplication. In the future, there some be some abstraction so that the two visitors could share these values/logic for updating them.
OK, can you add a FIXME explaining this, above their name or something like that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104751/new/

https://reviews.llvm.org/D104751