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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


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

----------------
jdoerfert wrote:
> 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.
Happy to rename to ModelEvaluator, but I would shy away from documenting how this may pan out if we wanted to support other evaluators. The issue would with the API typing. Currently it is tied to TF. If we wanted to generalize later over a few other evaluators, we'd need to figure out common abstractions and so on - time at which the name of the base type would be the least disruptive of the changes, I think.

Note also that this code isn't that generic, in that its compilation is tied to the presence of the tensorflow C API library - the only generic part of it is that it's reused by both the size estimator in this patch, and the development mode InlineAdvisor (next patch).

Back to the original feedback, happy to rename to ModelEvaluator (i.e. not even do "using", just rename); or we can keep it "TF" because it's tied to tensorflow anyway (APIs and implementation)

wdyt?


================
Comment at: llvm/include/llvm/Analysis/Utils/TFUtils.h:40
+                   const std::vector<std::string> &OutputNames,
+                   const char *Tags = "serve");
+  ~TFModelEvaluator();
----------------
jdoerfert wrote:
> 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.
"serve" is the typical tag used in tensorflow for the graph that's ready to be used ("served").

Re. strings - the usecase we'll have in the 'development' mode InlineAdvisor will require string concatenation, and I think at that point StringRef would loose its benefit.

The vectors will be larger (the development mode currently has 11 features, and we're looking at more) - would SmallVectorImpl still make sense?


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


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


================
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."));
+
----------------
jdoerfert wrote:
> Can we move this to the top, just below the `DEBUG_TYPE`. That is where people (=I) look for command line options.
OK - I flipped the ifdef, so this bubbled up, does that work?


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:65
+#include "llvm/IR/Instruction.def"
+}
+
----------------
jdoerfert wrote:
> We do not have an existing function for this? Surprising.
Not that I can tell - see also TargetLoweringBase.cpp InstructionOpcodes enum, and Instruction.h - the OtherOps enum. I would hesitate using OtherOps because it may evolve differently.


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:101
+    IRToNativeSizeLearning::FunctionFeatures::ImportantInstructionSuccessions =
+        {{1, 34},  {15, 27}, {53, 53}, {53, 34}, {1, 11},  {32, 2},  {2, 48},
+         {28, 48}, {1, 45},  {49, 32}, {57, 56}, {55, 53}, {1, 28},  {57, 34},
----------------
davidxl wrote:
> Is it possible to avoid using hardcoded opcodes?
Not easily, and the benefit would be unclear.

The pairs were generated through analysis of the data sets used for training this model. *If* we ended up down this path (i.e. feature pairs), they would be maintained in the same way - starting from analysis of data sets, not starting from IR. Using indices is just simpler in that case.


================
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}};
+
----------------
jdoerfert wrote:
> I guess this is basically fixed? maybe a `std::array` would do too.
> 
Yes, but (unless I'm missing something) I'd have to specify the "N" (the size). This way, I let the vector's size calculate other values (like FeatureCount)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82817