[PATCH] D88770: [MLInliner] Factor out logging

[Gaurav Jain via Phabricator via llvm-commits]

gjain requested changes to this revision.
gjain added a comment.
This revision now requires changes to proceed.

Very minor comments...



================
Comment at: llvm/include/llvm/Analysis/Utils/TFUtils.h:129
+
+  /// Construct a Logger. If IncludeReward is false, then logReward shouldn't
+  /// be called, and the reward feature won't be printed out.
----------------
Could you explain why the `IncludeReward` flag is necessary? What is the harm in calling `logReward`? Is there a alternative setup where we can avoid having this flag (perhaps 2 different classes)?


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:81
   /// DefaultDecision.
-  bool AdvisedDecision = false;
+  int64_t AdvisedDecision = false;
 
----------------
ditto


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:77
   /// What the default policy's decision would have been.
-  bool DefaultDecision = false;
+  int64_t DefaultDecision = false;
 
----------------
Do you mean to set this to 0?


================
Comment at: llvm/lib/Analysis/TFUtils.cpp:110
+
+  OutFile << "  feature_list: {\n";
+  OutFile << "    key: "
----------------
Indentation looks off here did you forget a closing curly from above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88770