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

[Jacob Hegna via Phabricator via llvm-commits]

jacobhegna added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineModelFeatureMaps.h:104
+    "exposed externally")                                                      \
+  M(CostEstimate, "cost_estimate", "total cost estimate (threshold - free)")
 
----------------
mtrofin wrote:
> why not leave this where it was? because you can now populate the remainder in a loop?
it's an artifact from when I was fiddling around with things - will move it back


================
Comment at: llvm/include/llvm/Analysis/InlineModelFeatureMaps.h:117
 #undef POPULATE_INDICES
-      NumberOfFeatures
+
+  NumberOfFeatures
----------------
mtrofin wrote:
> unrelated change?
are you referring to the newline that is inserted here? I think it just makes things easier to read in the enum


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104751