[PATCH] D83852: [llvm-profdata] Implement llvm-profdata overlap for sample profiles

[Wenlei He via Phabricator via llvm-commits]

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