[PATCH] D82817: [llvm] Native size estimator for training -Oz inliner

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Thanks for working on this. Glad we are making progress. Some comments below, mostly style, nothing major.



================
Comment at: llvm/include/llvm/Analysis/InlineSizeEstimatorAnalysis.h:36
+#endif // LLVM_ANALYSIS_INLINESIZEESTIMATORANALYSIS_H
\ No newline at end of file

----------------
I would prefer to remove the `TF` prefix (in the generic code parts). Since we only have a single model evaluator class we could add `using ModelEvaluator = TFModelEvaluator` here. We then document it saying that new evaluators might be added in which case ModelEvaluator becomes an interface implemented by different classes.


================
Comment at: llvm/include/llvm/Analysis/Utils/TFUtils.h:20
+
+bool ensureInitTF();
+
----------------
No undocumented top-level functions please. If this is tied to the class below, make it a documented static member.


================
Comment at: llvm/include/llvm/Analysis/Utils/TFUtils.h:29
+TFStatusPtr createTFStatus();
+TFSessionOptionsPtr createTFSessionOptions();
+
----------------
Same comment as above. Do we need these to populate the llvm namespace? Either way, we need documentation.


================
Comment at: llvm/include/llvm/Analysis/Utils/TFUtils.h:40
+                   const std::vector<std::string> &OutputNames,
+                   const char *Tags = "serve");
+  ~TFModelEvaluator();
----------------
Is "serve" a keyword for TF? If not I would recommend changing it. Could you please use llvm types if possible? The strings don't seem to be necessary, `StringRef` should do fine. For sure for the const char. `const SmallVectorImpl` should be preferred over `std::vector`. If you think a vector of strings is safer for the future, that is fine with me.


================
Comment at: llvm/include/llvm/Analysis/Utils/TFUtils.h:59
+  void initInput(int Index, TF_DataType Type,
+                 const std::vector<int64_t> &Dimensions);
+
----------------
Documentation.

Also below.


================
Comment at: llvm/include/llvm/Analysis/Utils/TFUtils.h:69
+  void deleteSession();
+  bool checkReportAndReset(const TF_Output &Output, StringRef Name);
+};
----------------
SmallVector's above if possible.


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:17
+#include <algorithm>
+#include <deque>
+
----------------
Nit: I think these go after the llvm includes.


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:44
+class TFModelEvaluator {};
+} // namespace llvm
+InlineSizeEstimatorAnalysis::InlineSizeEstimatorAnalysis() {}
----------------
Now that I see this, we might want to consider adding ModelEvaluator right away with some default implementation and deriving from it.


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:59
+    "ml-inliner-ir2native-model", cl::Hidden,
+    cl::desc("Path to saved model evaluating native size from IR."));
+
----------------
Can we move this to the top, just below the `DEBUG_TYPE`. That is where people (=I) look for command line options.


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:65
+#include "llvm/IR/Instruction.def"
+}
+
----------------
We do not have an existing function for this? Surprising.


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:120
+         {40, 34}, {1, 13},  {38, 34}, {29, 2},  {34, 2},  {1, 39},  {1, 22},
+         {1, 27},  {49, 1},  {1, 8},   {56, 2}};
+
----------------
I guess this is basically fixed? maybe a `std::array` would do too.



================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:158
+  }
+}
+
----------------
What is the difference between the opcode and the ID here? I mean why not use `I.getOpcode()` instead of this function?


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:167
+  return Ret;
+}
+
----------------
Are these functions (here and above) in global namespace and with external linkage? If possible, extend the anonymous namespace or make them static.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82817