[PATCH] D83852: [llvm-profdata] Implement llvm-profdata overlap for sample profiles
Wenlei He via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 1 14:31:59 PDT 2021
wenlei added inline comments.
================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1599
+ // in test profile.
+ BaseFuncProf.erase(Match);
+ }
----------------
aganea wrote:
> `Match` is invalidated after this line, so it cannot be compared with `BaseFuncProf.end()` afterwards at L1607 and L1609.
>
> In a Debug build on Windows/MSVC this asserts in MS-STL:
> {F18792732}
>
> The following tests fail because of this:
> ```
> LLVM :: tools/llvm-profdata/compact-sample.proftext
> LLVM :: tools/llvm-profdata/sample-overlap.test
> ```
>
> The following patch seems to fi the issue, but I thought I'll let you decide what to do?
> ```
> diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
> index 488dc8fa4317..38d9cb9461bb 100644
> --- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
> +++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
> @@ -1633,6 +1633,7 @@ void SampleOverlapAggregator::computeSampleProfileOverlap(raw_fd_ostream &OS) {
> "except inlinees");
> FuncOverlap.TestSample = TestStats[FuncOverlap.TestName].SampleSum;
>
> + bool Matched = false;
> const auto Match = BaseFuncProf.find(FuncOverlap.TestName);
> if (Match == BaseFuncProf.end()) {
> const FuncSampleStats &FuncStats = TestStats[FuncOverlap.TestName];
> @@ -1677,6 +1678,8 @@ void SampleOverlapAggregator::computeSampleProfileOverlap(raw_fd_ostream &OS) {
> // Remove matched base functions for later reporting functions not found
> // in test profile.
> BaseFuncProf.erase(Match);
> +
> + Matched = true;
> }
>
> // Print function-level similarity information if specified by options.
> @@ -1684,9 +1687,8 @@ void SampleOverlapAggregator::computeSampleProfileOverlap(raw_fd_ostream &OS) {
> "TestStats should have records for all functions in test profile "
> "except inlinees");
> if (TestStats[FuncOverlap.TestName].MaxSample >= FuncFilter.ValueCutoff ||
> - (Match != BaseFuncProf.end() &&
> - FuncOverlap.Similarity < LowSimilarityThreshold) ||
> - (Match != BaseFuncProf.end() && !FuncFilter.NameFilter.empty() &&
> + (Matched && FuncOverlap.Similarity < LowSimilarityThreshold) ||
> + (Matched && !FuncFilter.NameFilter.empty() &&
> FuncOverlap.BaseName.toString().find(FuncFilter.NameFilter) !=
> std::string::npos)) {
> assert(ProfOverlap.BaseSample > 0 &&
> ```
Good catch, thanks! I will fix it as you suggested.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83852/new/
https://reviews.llvm.org/D83852
More information about the llvm-commits
mailing list