[PATCH] D88770: [MLInliner] Factor out logging

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline 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.
----------------
gjain wrote:
> 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)?
I wouldn't subclass just on this aspect. To answer the question, the distinction is soft - in the inliner for size case, the reward won't be computed during the runtime of the compiler, so *maybe* it's easier to just add the whole field in post-processing. See my reply to Yundi's comment at DevelopmentModeInlineAdvisor.cpp:350


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:81
   /// DefaultDecision.
-  bool AdvisedDecision = false;
+  int64_t AdvisedDecision = false;
 
----------------
gjain wrote:
> ditto
yup, thanks!


================
Comment at: llvm/lib/Analysis/TFUtils.cpp:110
+
+  OutFile << "  feature_list: {\n";
+  OutFile << "    key: "
----------------
gjain wrote:
> Indentation looks off here did you forget a closing curly from above?
there's no curly, this is correct. I'll curly the llvm_unreachable, though, because I can see how their absence may cause confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88770