This is an archive of the discontinued LLVM Phabricator instance.

[InstrProf][Temporal] Add weight field to traces
ClosedPublic

Authored by ellis on Apr 12 2023, 11:00 AM.

Details

Summary

As discussed in [0], add a weight field to temporal profiling traces found in profiles. This allows users to use the --weighted-input= flag in the llvm-profdata merge command to weight traces from different scenarios differently.

Note that this is a breaking change, but since [1] landed very recently and there is no way to "use" this trace data, there should be no users of this feature. We believe it is acceptable to land this change without bumping the profile format version.

[0] https://reviews.llvm.org/D147812#4259507
[1] https://reviews.llvm.org/D147287

Diff Detail

Event Timeline

ellis created this revision.Apr 12 2023, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2023, 11:00 AM
ellis edited the summary of this revision. (Show Details)Apr 12 2023, 11:05 AM
ellis published this revision for review.Apr 12 2023, 11:07 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 12 2023, 11:07 AM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
snehasish added inline comments.Apr 12 2023, 1:15 PM
llvm/docs/CommandGuide/llvm-profdata.rst
203

typo: use

208

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.

213

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

llvm/include/llvm/ProfileData/InstrProfReader.h
403–404

nit: Prefer llvm::ArrayRef<TemporalProfTraceTy> instead of a constant reference?

llvm/tools/llvm-profdata/llvm-profdata.cpp
1326

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.

ellis marked 2 inline comments as done.Apr 12 2023, 4:48 PM
ellis added inline comments.
llvm/docs/CommandGuide/llvm-profdata.rst
208

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.

213

This simplifies a few things. Thanks for the suggestion!

llvm/include/llvm/ProfileData/InstrProfReader.h
403–404

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!

ellis updated this revision to Diff 513014.Apr 12 2023, 4:48 PM
ellis marked an inline comment as done.

Remove --temporal-profile-trace-weight flag

ellis edited the summary of this revision. (Show Details)Apr 12 2023, 4:49 PM
snehasish accepted this revision.Apr 12 2023, 5:08 PM

lgtm

llvm/docs/CommandGuide/llvm-profdata.rst
208

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.

llvm/lib/ProfileData/InstrProfWriter.cpp
314

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);
This revision is now accepted and ready to land.Apr 12 2023, 5:08 PM
ellis updated this revision to Diff 513030.Apr 12 2023, 6:08 PM
ellis marked 2 inline comments as done.

Rename flag to -temporal-profile-max-trace-length

llvm/lib/ProfileData/InstrProfWriter.cpp
314

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.

snehasish accepted this revision.Apr 12 2023, 8:17 PM
snehasish added inline comments.
llvm/lib/ProfileData/InstrProfWriter.cpp
314

Ah sorry for the misleading comment then, I should have read the source more closely.

This revision was landed with ongoing or failed builds.Apr 13 2023, 10:37 AM
This revision was automatically updated to reflect the committed changes.
ellis added inline comments.Apr 13 2023, 11:33 AM
llvm/unittests/ProfileData/InstrProfTest.cpp
252

This broke some builds because of compiler warnings, but I believe it should be fixed by https://reviews.llvm.org/D148259.

https://lab.llvm.org/buildbot/#/builders/57/builds/26047

/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/unittests/ProfileData/InstrProfTest.cpp:252:3: error: 'SmallVector' may not intend to support class template argument deduction [-Werror,-Wctad-maybe-unsupported]
  SmallVector Traces = {LargeTrace, SmallTrace};
  ^
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include/llvm/ADT/SmallVector.h:1199:22: note: add a deduction guide to suppress this warning
class LLVM_GSL_OWNER SmallVector : public SmallVectorImpl<T>,