[PATCH] D88770: [MLInliner] Factor out logging

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:116-118
+  size_t OutputCount = 1;
+  size_t DefaultDecisionPos = 0;
+  size_t DecisionPos = 0;
----------------
yundiqian wrote:
> initialize them as 0, -1, -1?
OutputCount is at least 1, actually. Not sure what setting Pos values to max_uint64 would do - I guess it'd be checking we set them. OK.


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:347
+      FT, TensorSpec::createSpec<int64_t>(RewardName, {1}),
+      InlineSizeEstimatorAnalysis::isEvaluatorRequested());
 }
----------------
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.


================
Comment at: llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:369-370
+
+  L->logTensorValue(DefaultDecisionPos, &Event.DefaultDecision);
+  L->logTensorValue(DecisionPos, &Event.AdvisedDecision);
+  if (InlineSizeEstimatorAnalysis::isEvaluatorRequested())
----------------
yundiqian wrote:
> It seems that we don't need DefaultDecisionPos and DecisionPos by having logTensorValue(CurrentFeature++, &Event....) cause you are making assumptions anyway? 
Either or. Let me add an assertion here that we got at the right place - that's what I wanted to use them for, really.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88770