[PATCH] D83733: [llvm] Development-mode InlineAdvisor

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:30
+
+static cl::opt<std::string> TFTrainedModelPath(
+    "ml-inliner-trained-model", cl::Hidden,
----------------
davidxl wrote:
> mtrofin wrote:
> > davidxl wrote:
> > > TrainedModel sounds misleading as if it has been trained. Perhaps just TrainModel or ModelInTrain? Or TrainingModel?
> > It's actually correct, it is trained: this would be a model obtained from a previous iteration of the reinforcement learning training loop. We aren't mutating it here.
> As a reader, I feel the name means 'fully trained' instead of being 'partially trained' in the iteration loop.
renamed


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:258
+/// sacrificed for ease of use while training.
+class DynamicModelRunner final : public MLModelRunner {
+public:
----------------
davidxl wrote:
> mtrofin wrote:
> > davidxl wrote:
> > > Perhaps change DynamicModel name to be TrainModel to make the name consistent with the option.
> > I don't want to suggest it's being trained, though. How about LoadableModelRunner - the key aspect of this ModelRunner is that the model may be loaded from command line;
> > 
> > or, to contrast the release/AOT case ('ReleaseModeModelRunner) - DevelopmentModeModelRunner
> > 
> > wdyt?
> How about IterativeModelRunner?
ModelUnderTrainingRunner? similar to how I renamed the option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83733