[PATCH] D142168: [mlgo] Stream the training data

[Kazu Hirata via Phabricator via llvm-commits]

kazu accepted this revision.
kazu added a comment.
This revision is now accepted and ready to land.

LGTM.  Just a minor comment about `currentObservationID`.



================
Comment at: llvm/include/llvm/Analysis/Utils/TrainingLogger.h:122
 
-  // Warning! For int32_t, the return is set up for int64_t, so the caller needs
-  // to piecemeal cast their int32_t values.
-  // FIXME: let's drop int32_t support. While it's supported by evaluator, it's
-  // not supported by the tensorflow::SequenceExample proto. For small values,
-  // we can consider using bytes.
-  char *addEntryAndGetFloatOrInt64Buffer(size_t FeatureID);
+  std::optional<size_t> currentObservationID() const {
+    auto I = ObservationIDs.find(CurrentContext);
----------------
If you only care about `currentObservationID().has_value()`, I wonder if you can rename this function to `isObservationInProgress` or something and do:

```
  return ObservationIDs.count(CurrentContext);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142168