Skip to content

[BOLT][NFCI] Skip validation in parseLBRSample #143288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

aaupov
Copy link
Contributor

@aaupov aaupov commented Jun 7, 2025

Parsed branches and fall-throughs are validated in doBranch and
doTrace respectively. Simplify parseLBRSample by omitting the
validation. This also speeds up perf data processing as checks are only
done once for aggregated branches/fall-throughs and not individual LBR
entries.

Since invalid/external addresses are no longer sanitized during parsing,
sanitize them in doBranch.

Test Plan: NFC

aaupov added 3 commits June 7, 2025 14:48
Created using spr 1.3.4
Created using spr 1.3.4
@aaupov aaupov marked this pull request as ready for review June 7, 2025 22:11
@llvmbot llvmbot added the BOLT label Jun 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 7, 2025

@llvm/pr-subscribers-bolt

Author: Amir Ayupov (aaupov)

Changes

Parsed branches and fall-throughs are validated in doBranch and
doTrace respectively. Simplify parseLBRSample by omitting the
validation. This also speeds up perf data processing as checks are only
done once for aggregated branches/fall-throughs and not individual LBR
entries.

Test Plan: NFC


Full diff: https://github.com/llvm/llvm-project/pull/143288.diff

1 Files Affected:

  • (modified) bolt/lib/Profile/DataAggregator.cpp (+5-43)
diff --git a/bolt/lib/Profile/DataAggregator.cpp b/bolt/lib/Profile/DataAggregator.cpp
index b1172fd13bc72..addff196f4f5b 100644
--- a/bolt/lib/Profile/DataAggregator.cpp
+++ b/bolt/lib/Profile/DataAggregator.cpp
@@ -1425,54 +1425,16 @@ void DataAggregator::parseLBRSample(const PerfBranchSample &Sample,
       const uint64_t TraceTo = NextLBR->From;
       const BinaryFunction *TraceBF =
           getBinaryFunctionContainingAddress(TraceFrom);
-      if (opts::HeatmapMode == opts::HeatmapModeKind::HM_Exclusive) {
-        FTInfo &Info = FallthroughLBRs[Trace(TraceFrom, TraceTo)];
+      FTInfo &Info = FallthroughLBRs[Trace(TraceFrom, TraceTo)];
+      if (TraceBF && TraceBF->containsAddress(LBR.From))
         ++Info.InternCount;
-      } else if (TraceBF && TraceBF->containsAddress(TraceTo)) {
-        FTInfo &Info = FallthroughLBRs[Trace(TraceFrom, TraceTo)];
-        if (TraceBF->containsAddress(LBR.From))
-          ++Info.InternCount;
-        else
-          ++Info.ExternCount;
-      } else {
-        const BinaryFunction *ToFunc =
-            getBinaryFunctionContainingAddress(TraceTo);
-        if (TraceBF && ToFunc) {
-          LLVM_DEBUG({
-            dbgs() << "Invalid trace starting in " << TraceBF->getPrintName()
-                   << formatv(" @ {0:x}", TraceFrom - TraceBF->getAddress())
-                   << formatv(" and ending @ {0:x}\n", TraceTo);
-          });
-          ++NumInvalidTraces;
-        } else {
-          LLVM_DEBUG({
-            dbgs() << "Out of range trace starting in "
-                   << (TraceBF ? TraceBF->getPrintName() : "None")
-                   << formatv(" @ {0:x}",
-                              TraceFrom - (TraceBF ? TraceBF->getAddress() : 0))
-                   << " and ending in "
-                   << (ToFunc ? ToFunc->getPrintName() : "None")
-                   << formatv(" @ {0:x}\n",
-                              TraceTo - (ToFunc ? ToFunc->getAddress() : 0));
-          });
-          ++NumLongRangeTraces;
-        }
-      }
+      else
+        ++Info.ExternCount;
       ++NumTraces;
     }
     NextLBR = &LBR;
 
-    // Record branches outside binary functions for heatmap.
-    if (opts::HeatmapMode == opts::HeatmapModeKind::HM_Exclusive) {
-      TakenBranchInfo &Info = BranchLBRs[Trace(LBR.From, LBR.To)];
-      ++Info.TakenCount;
-      continue;
-    }
-    uint64_t From = getBinaryFunctionContainingAddress(LBR.From) ? LBR.From : 0;
-    uint64_t To = getBinaryFunctionContainingAddress(LBR.To) ? LBR.To : 0;
-    if (!From && !To)
-      continue;
-    TakenBranchInfo &Info = BranchLBRs[Trace(From, To)];
+    TakenBranchInfo &Info = BranchLBRs[Trace(LBR.From, LBR.To)];
     ++Info.TakenCount;
     Info.MispredCount += LBR.Mispred;
   }

aaupov added 3 commits June 7, 2025 20:32
Created using spr 1.3.4
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
aaupov added 2 commits June 8, 2025 17:44
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@aaupov aaupov changed the base branch from users/aaupov/spr/main.boltnfci-skip-validation-in-parselbrsample to main June 9, 2025 00:44
@aaupov aaupov merged commit 03bbd04 into main Jun 9, 2025
8 of 13 checks passed
@aaupov aaupov deleted the users/aaupov/spr/boltnfci-skip-validation-in-parselbrsample branch June 9, 2025 00:50
kaadam added a commit to kaadam/llvm-project that referenced this pull request Jun 10, 2025
The test could be simplified after llvm#143288 PR since
the validation phase is removed from parseLBRSample.
Now we can use branchLBRs container for the testing.
Formerly if Bolt was supplied with mock addresses, branchLBRs container
was empty due to validation phase.
rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 2025
Parsed branches and fall-throughs are validated in `doBranch` and
`doTrace` respectively. Simplify parseLBRSample by omitting the
validation. This also speeds up perf data processing as checks are only
done once for aggregated branches/fall-throughs and not individual LBR
entries.

Since invalid/external addresses are no longer sanitized during parsing,
sanitize them in `doBranch`.

Test Plan: updated X86/pre-aggregated-perf.test
DhruvSrivastavaX pushed a commit to DhruvSrivastavaX/lldb-for-aix that referenced this pull request Jun 12, 2025
Parsed branches and fall-throughs are validated in `doBranch` and
`doTrace` respectively. Simplify parseLBRSample by omitting the
validation. This also speeds up perf data processing as checks are only
done once for aggregated branches/fall-throughs and not individual LBR
entries.

Since invalid/external addresses are no longer sanitized during parsing,
sanitize them in `doBranch`.

Test Plan: updated X86/pre-aggregated-perf.test
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
Parsed branches and fall-throughs are validated in `doBranch` and
`doTrace` respectively. Simplify parseLBRSample by omitting the
validation. This also speeds up perf data processing as checks are only
done once for aggregated branches/fall-throughs and not individual LBR
entries.

Since invalid/external addresses are no longer sanitized during parsing,
sanitize them in `doBranch`.

Test Plan: updated X86/pre-aggregated-perf.test
kaadam added a commit to kaadam/llvm-project that referenced this pull request Jun 16, 2025
The test could be simplified after llvm#143288 PR since
the validation phase is removed from parseLBRSample.
Now we can use branchLBRs container for the testing.
Formerly if Bolt was supplied with mock addresses, branchLBRs container
was empty due to validation phase.
kaadam added a commit to kaadam/llvm-project that referenced this pull request Jun 19, 2025
The test could be simplified after llvm#143288 PR since
the validation phase is removed from parseLBRSample.
Now we can use branchLBRs container for the testing.
Formerly if Bolt was supplied with mock addresses, branchLBRs container
was empty due to validation phase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants