[PATCH] D148150: [InstrProf][Temporal] Add weight field to traces

[Ellis Hoag via Phabricator via llvm-commits]

ellis marked 2 inline comments as done.
ellis added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-profdata.rst:208
+
+.. option:: --max-temporal-profile-trace-length
+
----------------
snehasish wrote:
> nit: Should we rename this flag to --temporal-profile-max-trace-length to be consistent with the naming of the others with the same prefix? I missed this in the prior review.
I think of this as "max-X-length" where X is "temporal-profile-trace". I'm ok with either, so let me know if you have a strong opinion.


================
Comment at: llvm/docs/CommandGuide/llvm-profdata.rst:213
+
+.. option:: --temporal-profile-trace-weight
+
----------------
snehasish wrote:
> I think in this usage model only a single trace with unique weight can be merged at a time. Can we use existing -weighted-input flag in llvm-profdata to implement this?
> https://llvm.org/docs/CommandGuide/llvm-profdata.html#cmdoption-llvm-profdata-merge-weighted-input
This simplifies a few things. Thanks for the suggestion!


================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:403
 
-  const SmallVector<TemporalProfTraceTy> &getTemporalProfTraces() override;
+  const SmallVector<TemporalProfTraceTy> &
+  getTemporalProfTraces(std::optional<uint64_t> Weight = {}) override;
----------------
snehasish wrote:
> nit: Prefer llvm::ArrayRef<TemporalProfTraceTy> instead of a constant reference?
I actually realized I should remove the `const` and make the traces in `addTemporalProfileTraces()` also a reference to avoid a copy. Thanks for pointing this out!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148150