[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/InlineCost.h:57
 
+// The features which determine the computation of InlineCost.
+#define INLINE_COST_FEATURE_ITERATOR(M)                                        \
----------------
I think it'd be preferable if this were defined on the ML side, like in InlineModelFeatureMaps.h.

The reason is that it brings in this concept of feature name (e.g. "sroa_savings"), which is a ML side concept.


================
Comment at: llvm/include/llvm/Analysis/InlineModelFeatureMaps.h:17
 
+#include "llvm/Analysis/InlineCost.h"
+
----------------
this wouldn't be needed once you move in the changes in InlineCost.h here.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:469
+
+    if (isExclusiveMLFeature(Feature)) {
+      return;
----------------
I don't think the ML aspects should intermingle with the non-ml ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104751