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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/cmake/modules/TensorFlowCompile.cmake:26
+  # grouped into one target.
+  set(GENERATED_OBJS ${GENERATED_OBJS} ${CMAKE_CURRENT_BINARY_DIR}/${fname}.o PARENT_SCOPE)
+  set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/${fname}.o PROPERTIES
----------------
there are lots of references to ${CMAKE_CURRENT_BINARY_DIR}/${fname} here, perhaps use a common variable for it?


================
Comment at: llvm/include/llvm/Analysis/InlineModelFeatureMaps.h:57
+
+static constexpr size_t NumberOfFeatures =
+    static_cast<size_t>(FeatureIndex::NumberOfFeatures);
----------------
put these static variables inside the .cpp file to avoid multiple copies if the header is included by different sources.


================
Comment at: llvm/include/llvm/Analysis/InlineModelRunner.h:21
+/// tensorflow "saved model".
+class InlineModelRunner {
+public:
----------------
Is this interface just for inliner or more general. Perhaps just name it MLModelRunner?


================
Comment at: llvm/lib/Analysis/MLInlineAdvisor.cpp:37
+
+static cl::opt<float> SizeIncreaseThreshold(
+    "ml-advisor-size-increase-threshold", cl::Hidden,
----------------
Is this for controlling training time overhead?


================
Comment at: llvm/lib/Analysis/MLInlineAdvisor.cpp:72
+      Function *F = CGNode->getFunction();
+      if (F && !F->isDeclaration())
+        for (auto &I : instructions(F)) {
----------------
reverse the condition with continue to reduce nesting level


================
Comment at: llvm/lib/Analysis/MLInlineAdvisor.cpp:77
+            auto Pos = FunctionLevels.find(Called);
+            // In bottom up traversal, an inlinable call is either in the
+            // same SCC, or to a function in a visited SCC. So not finding its
----------------
inlinable callee


================
Comment at: llvm/lib/Analysis/ReleaseModeModelRunner.cpp:31
+/// SavedModel for efficient execution.
+class ReleaseModeModelRunner final : public InlineModelRunner {
+public:
----------------
MLInferenceRunner?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81515