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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-profdata.rst:203
+ The maximum number of temporal profile traces to be stored in the output
+ profile. If more traces are added, we will used reservoir sampling to select
+ which traces to keep. Note that changing this value between different merge
----------------
typo: use


================
Comment at: llvm/docs/CommandGuide/llvm-profdata.rst:208
+
+.. option:: --max-temporal-profile-trace-length
+
----------------
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.


================
Comment at: llvm/docs/CommandGuide/llvm-profdata.rst:213
+
+.. option:: --temporal-profile-trace-weight
+
----------------
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


================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:403
 
-  const SmallVector<TemporalProfTraceTy> &getTemporalProfTraces() override;
+  const SmallVector<TemporalProfTraceTy> &
+  getTemporalProfTraces(std::optional<uint64_t> Weight = {}) override;
----------------
nit: Prefer llvm::ArrayRef<TemporalProfTraceTy> instead of a constant reference?


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1332
   if (ProfileKind == instr)
     mergeInstrProfile(WeightedInputs, DebugInfoFilename, Remapper.get(),
                       OutputFilename, OutputFormat,
----------------
You should be able to extract the weight for an input (in this case, a raw temporal prof trace) from this instead of adding a separate flag. Also see the comment above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148150