[PATCH] D88770: [MLInliner] Factor out logging

[Yundi Qian via Phabricator via llvm-commits]

yundiqian accepted this revision.
yundiqian added inline comments.


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:347
+      FT, TensorSpec::createSpec<int64_t>(RewardName, {1}),
+      InlineSizeEstimatorAnalysis::isEvaluatorRequested());
 }
----------------
mtrofin wrote:
> yundiqian wrote:
> > to confirm, it will still not have delta_size field? not a default value 0?
> It's up to the user. In this case, it won't. If the user needs to output a reward field, they can, and set it to 0 (except the last one) if it's for a final reward algo. 
> 
> My thinking was that, since in this case we insert the final reward after compilation, when we measure the size, then maybe it'd be easier not to have the field in the first place. 
> 
> Let me know if it'd be easier to always have the field there instead.
yeah, I agree, not having it makes sense


================
Comment at: llvm/lib/Analysis/TFUtils.cpp:402-406
+  size_t Tensor0Size = FeatureSpecs[0].Spec.getElementCount() *
+                       FeatureSpecs[0].Spec.getElementByteSize();
+  size_t NumberOfRecords = RawLogData[0].size() / Tensor0Size;
+  if (NumberOfRecords == 0)
+    return;
----------------
mtrofin wrote:
> yundiqian wrote:
> > shall we also add a check here making sure all features are of equal length? we have no check about Logger is used in its intended way.
> That'd be an assert, not a runtime check, because it'd be a bug; it'd also be detectable by code further below failing. In other words, should be detected by testing.
ok, it's fine if this is covered by test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88770