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

[Snehasish Kumar via Phabricator via llvm-commits]

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

lgtm



================
Comment at: llvm/docs/CommandGuide/llvm-profdata.rst:208
+
+.. option:: --max-temporal-profile-trace-length
+
----------------
ellis wrote:
> 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.
I think moving "max" after the "temporal-profile-trace" (or even "temporal-profile") prefix does not change the meaning, so having a consistent prefix for the options for this feature would be nice in my opinion. 


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:314
 void InstrProfWriter::addTemporalProfileTraces(
-    SmallVector<TemporalProfTraceTy> SrcTraces, uint64_t SrcStreamSize) {
+    SmallVectorImpl<TemporalProfTraceTy> &SrcTraces, uint64_t SrcStreamSize) {
   // Assume that the source has the same reservoir size as the destination to
----------------
If you use llvm::ArrayRef instead of SmallVectorImpl& then in the unit tests you can avoid having to create SmallVector objects which are then passed to this function. You can replace them with initializer lists and simplify the code a little bit.

```
SmallVector Traces1({FooTrace}), Traces2({BarTrace});
Writer.addTemporalProfileTraces(Traces1, 1);
Writer2.addTemporalProfileTraces(Traces2, 1);
```
becomes
```
Writer.addTemporalProfileTraces({FooTrace}, 1);
Writer2.addTemporalProfileTraces({BarTrace}, 1);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148150