[PATCH] D81515: [llvm] Release-mode ML InlineAdvisor

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

Can you also rebase the patch?



================
Comment at: llvm/CMakeLists.txt:964
 
+set(TENSORFLOW_AOT_PATH "Path to TensorFlow pip install dir" CACHE PATH
+  "Path to TensorFlow pip install dir")
----------------
Add a comment here describing briefly how to download TF packages and set AOT_PATH?


================
Comment at: llvm/cmake/modules/TensorFlowCompile.cmake:1
+function(tfcompile model tag_set signature_def_key fname cpp_class)
+  if (IS_ABSOLUTE ${model})
----------------
document the function.


================
Comment at: llvm/cmake/modules/TensorFlowCompile.cmake:19
+  # Aggregate the objects so that results of different tfcompile calls may be
+  # groupped into one target.
+  set(GENERATED_OBJS ${GENERATED_OBJS} ${CMAKE_CURRENT_BINARY_DIR}/${fname}.o PARENT_SCOPE)
----------------
groupped -- grouped.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:159
+    Advisor = llvm::getReleaseModeAdvisor(M, MAM);
+#endif
     break;
----------------
add an assert in the #else branch?


================
Comment at: llvm/lib/Analysis/ML/Common/MLInlineAdvisor.cpp:62
+
+  for (auto I = scc_begin(CG.get()); !I.isAtEnd(); ++I) {
+    const std::vector<CallGraphNode *> &CGNodes = *I;
----------------
Add comments here what it does -- e.g, feature extraction etc.


================
Comment at: llvm/lib/Analysis/ML/Common/MLInlineAdvisor.cpp:105
+
+void MLInlineAdvisor::onSuccessfulInlining(const MLInlineAdvice &Advice,
+                                           bool CalleeWasDeleted) {
----------------
add top level comments documenting what it does.


================
Comment at: llvm/lib/Analysis/ML/Common/MLInlineAdvisor.cpp:206
+
+  ModelRunner->set_feature(FeatureIndex::CalleeBasicBlockCount,
+                           CalleeBefore.BasicBlockCount);
----------------
extract the feature extraction code into a small helper?


================
Comment at: llvm/lib/Analysis/ML/InlineModelFeatureMaps.h:18
+enum class FeatureIndex : size_t {
+  CalleeBasicBlockCount = 0,
+  CallSiteHeight,
----------------
add comment on each feature


================
Comment at: llvm/lib/Analysis/ML/InlineModelFeatureMaps.h:38
+
+static const std::array<std::string, NumberOfFeatures> FeatureNameMap{
+    /*FeatureList::CalleeBasicBlockCount*/ "callee_basic_block_count",
----------------
It is hard to keep the index in sync with names. how about something with a def table:

// ml_features.def

DEFINE_FEATURE(CalleeBasicCount, "callee bb count")
DEFINE_FEATURE(CallSiteHeight, "callsite_height)
.....

For enum define
#define DEFINE_FEATURE(en, name) en,
#include "ml_features.def"
#undef DEFINE_FEATURE


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81515