[PATCH] D88770: [MLInliner] Factor out logging

[Yundi Qian via Phabricator via llvm-commits]

yundiqian added inline comments.


================
Comment at: llvm/include/llvm/Analysis/Utils/TFUtils.h:106-120
+/// The assumption is that, for an event to be logged (i.e. a set of feature
+/// values and a reward), the user calls the log* API for each feature exactly
+/// once, providing the index matching the position in the feature spec list
+/// provided at construction:
+/// event 0:
+///   logTensorValue(0, ...)
+///   logTensorValue(1, ...)
----------------
add the documentation about calling print() after the trace is finished


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:116-118
+  size_t OutputCount = 1;
+  size_t DefaultDecisionPos = 0;
+  size_t DecisionPos = 0;
----------------
initialize them as 0, -1, -1?


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:347
+      FT, TensorSpec::createSpec<int64_t>(RewardName, {1}),
+      InlineSizeEstimatorAnalysis::isEvaluatorRequested());
 }
----------------
to confirm, it will still not have delta_size field? not a default value 0?


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:369-370
+
+  L->logTensorValue(DefaultDecisionPos, &Event.DefaultDecision);
+  L->logTensorValue(DecisionPos, &Event.AdvisedDecision);
+  if (InlineSizeEstimatorAnalysis::isEvaluatorRequested())
----------------
It seems that we don't need DefaultDecisionPos and DecisionPos by having logTensorValue(CurrentFeature++, &Event....) cause you are making assumptions anyway? 


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88770