Address reviewers 2

Author: kaadam

bolt/lib/Profile/DataAggregator.cpp

@@ -49,9 +49,7 @@ static cl::opt<bool>
                      cl::desc("aggregate basic samples (without LBR info)"),
                      cl::cat(AggregatorCategory));
 
-cl::opt<bool> ArmSPE("spe",
-                     cl::desc("Enable Arm SPE mode. Can combine with `--nl` "
-                              "to use in no-lbr mode"),
+cl::opt<bool> ArmSPE("spe", cl::desc("Enable Arm SPE mode."),
                      cl::cat(AggregatorCategory));
 
 static cl::opt<std::string>
@@ -187,7 +185,10 @@ void DataAggregator::start() {
   findPerfExecutable();
 
   if (opts::ArmSPE) {
-    // pid    from_ip      to_ip        predicted/missed not-taken?
+    // pid    from_ip      to_ip        flags
+    // where flags could be:
+    // P/M: whether branch was Predicted or Mispredicted.
+    // N: optionally appears when the branch was Not-Taken (ie fall-through)
     // 12345  0x123/0x456/PN/-/-/8/RET/-
     launchPerfProcess("SPE brstack events", MainEventsPPI,
                       "script -F pid,brstack --itrace=bl",
@@ -1004,7 +1005,8 @@ ErrorOr<DataAggregator::LBREntry> DataAggregator::parseLBREntry() {
   if (std::error_code EC = MispredStrRes.getError())
     return EC;
   StringRef MispredStr = MispredStrRes.get();
-  // SPE brstack mispredicted flags might be two characters long: 'PN' or 'MN'.
+  // SPE brstack mispredicted flags might be up to two characters long:
+  // 'PN' or 'MN'. Where 'N' optionally appears.
   bool ValidStrSize = opts::ArmSPE
                           ? MispredStr.size() >= 1 && MispredStr.size() <= 2
                           : MispredStr.size() == 1;
@@ -1521,7 +1523,7 @@ void DataAggregator::printBranchStacksDiagnostics(
 std::error_code DataAggregator::parseBranchEvents() {
   std::string BranchEventTypeStr =
       !opts::ArmSPE ? "branch events" : "SPE branch events in LBR-format";
-  outs() << "PERF2BOLT: " << BranchEventTypeStr << "...\n";
+  outs() << "PERF2BOLT: parse " << BranchEventTypeStr << "...\n";
   NamedRegionTimer T("parseBranch", "Parsing " + BranchEventTypeStr,
                      TimerGroupName, TimerGroupDesc, opts::TimeAggregator);
 
@@ -1582,8 +1584,11 @@ std::error_code DataAggregator::parseBranchEvents() {
       else
         errs()
             << "PERF2BOLT-WARNING: all recorded samples for this binary lack "
-               "SPE brstack entries. Record profile with:"
-               "perf record arm_spe_0/branch_filter=1/";
+               "SPE brstack entries. The minimum required version of "
+               "Linux-perf is v6.14 or higher for brstack support. "
+               "With an older Linux-perf you may get zero samples. "
+               "Plese also make sure about you recorded profile with: "
+               "perf record -e 'arm_spe_0/branch_filter=1/'.";
     } else {
       printBranchStacksDiagnostics(NumTotalSamples - NumSamples);
     }

bolt/test/perf2bolt/AArch64/perf2bolt-spe.test

@@ -2,6 +2,8 @@
 
 REQUIRES: system-linux,perf,target=aarch64{{.*}}
 
+RUN: %clang %cflags %p/../../Inputs/asm_foo.s %p/../../Inputs/asm_main.c -o %t.exe
+
 RUN: perf record -e cycles -q -o %t.perf.data -- %t.exe 2> /dev/null
 
 RUN: (perf2bolt -p %t.perf.data -o %t.perf.boltdata --spe %t.exe 2> /dev/null; exit 0) | FileCheck %s --check-prefix=CHECK-SPE-LBR

bolt/test/perf2bolt/X86/perf2bolt-spe.test

@@ -4,6 +4,6 @@ REQUIRES: system-linux,x86_64-linux
 
 RUN: %clang %cflags %p/../../Inputs/asm_foo.s %p/../../Inputs/asm_main.c -o %t.exe
 RUN: touch %t.empty.perf.data
-RUN: not perf2bolt -p %t.empty.perf.data -o %t.perf.boltdata --nl --spe --pa %t.exe 2>&1 | FileCheck %s
+RUN: not perf2bolt -p %t.empty.perf.data -o %t.perf.boltdata --spe --pa %t.exe 2>&1 | FileCheck %s
 
 CHECK: perf2bolt: -spe is available only on AArch64.

bolt/unittests/Profile/PerfSpeEvents.cpp

@@ -73,13 +73,13 @@ struct PerfSpeEventsTestHelper : public testing::Test {
   std::unique_ptr<ObjectFile> ObjFile;
   std::unique_ptr<BinaryContext> BC;
 
-  /// Compare LBREntries
+  // @return true if LBREntries are equal.
   bool checkLBREntry(const LBREntry &Lhs, const LBREntry &Rhs) {
     return Lhs.From == Rhs.From && Lhs.To == Rhs.To &&
            Lhs.Mispred == Rhs.Mispred;
   }
 
-  /// Parse and check SPE brstack as LBR
+  // Parse and check SPE brstack as LBR.
   void parseAndCheckBrstackEvents(
       uint64_t PID,
       const std::vector<SmallVector<LBREntry, 2>> &ExpectedSamples) {
@@ -102,12 +102,9 @@ struct PerfSpeEventsTestHelper : public testing::Test {
       EXPECT_EQ(Sample.LBR.size(), ExpectedSamples[NumSamples].size());
 
       // Check the parsed LBREntries.
-      const auto *ActualIter = Sample.LBR.begin();
-      const auto *ExpectIter = ExpectedSamples[NumSamples].begin();
-      while (ActualIter != Sample.LBR.end() &&
-             ExpectIter != ExpectedSamples[NumSamples].end())
-        EXPECT_TRUE(checkLBREntry(*ActualIter++, *ExpectIter++));
-
+      for (auto [Actual, Expected] :
+           zip_equal(Sample.LBR, ExpectedSamples[NumSamples]))
+        EXPECT_TRUE(checkLBREntry(Actual, Expected));
       ++NumSamples;
     }
   }
@@ -135,7 +132,7 @@ TEST_F(PerfSpeEventsTestHelper, SpeBranchesWithBrstack) {
                          "  1234  0xe001/0xe002/P/-/-/14/RET/-\n"
                          "  1234  0xf001/0xf002/MN/-/-/8/COND/-\n";
 
-  std::vector<SmallVector<LBREntry, 2>> ExpectedSamples = {
+  std::vector<SmallVector<LBREntry>> ExpectedSamples = {
       {{{0xa001, 0xa002, false}}}, {{{0xb001, 0xb002, false}}},
       {{{0xc001, 0xc002, false}}}, {{{0xd001, 0xd002, true}}},
       {{{0xe001, 0xe002, false}}}, {{{0xf001, 0xf002, true}}},