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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish accepted this revision.
snehasish added inline comments.


================
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
----------------
ellis wrote:
> snehasish wrote:
> > 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);
> > ```
> > 
> I can't use `ArrayRef` because I'm modifying `SrcTraces` by moving traces from it to the destination writer. `MutableArrayRef` causes subtle bugs in the unit tests when I make the above change because it clears `FooTrace` and `BarTrace`. I think I could use `OwningArrayRef` but I think this would lead to more copying than is necessary.
Ah sorry for the misleading comment then, I should have read the source more closely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148150