[PATCH] D85674: [MLInliner] In development mode, obtain the output specs from a file

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 3 inline comments as done.
mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:63-67
+static cl::opt<std::string> TFOutputSpecOverride(
+    "ml-inliner-output-spec-override", cl::Hidden,
+    cl::desc("Override the path to the output spec json file. See "
+             "-ml-inliner-model-under-training documentation for the "
+             "specification of that file."));
----------------
yundiqian wrote:
> what's the motivation of providing this override alternative?
Testing. See the added run in development-training-log.ll


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:656-669
+  ModelUnderTrainingRunner *MUTRPtr = nullptr;
   bool IsDoingInference = false;
   if (TFModelUnderTrainingPath.empty())
     Runner.reset(new NoInferenceModelRunner(Ctx));
   else {
-    Runner = std::make_unique<ModelUnderTrainingRunner>(
+    auto MUTR = std::make_unique<ModelUnderTrainingRunner>(
         Ctx, TFModelUnderTrainingPath);
----------------
yundiqian wrote:
> I'm a bit confused here, why we need both MUTR and Runner in the TrainingLogger? Are they just the same? (why not using ModelRunner instead of MUTR in all TrainingLogger functions?)
Runner is generically typed as MLModelRunner, which could be either a ModelUnderTrainingRunner or the NoInferenceModelRunner, see lines 659 and 661.


================
Comment at: llvm/lib/Analysis/models/inliner/output_spec.json:1-13
+[
+    {
+        "logging_name": "inlining_decision",
+        "tensor_spec": {
+            "name": "StatefulPartitionedCall",
+            "port": 0,
+            "type": "int64",
----------------
yundiqian wrote:
> where is this file being used? Is it used when running default policy?
It is used alongside the pre-trained policy, for example in one of the current runs in tests (incl. development-training-log.ll's current test that uses that model. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85674