[PATCH] D115538: [CSSPGO][llvm-profgen] Fix external address issues of perf reader (leading external LBR part)

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:624
+        NumTopExternalFrame++;
+        CallStack.emplace_back(UnwindState::ProfiledFrame::ExternalAddr);
+      }
----------------
wlei wrote:
> wenlei wrote:
> > If only leaf frames are external, can we keep the remaining frames for unwinding?
> Sounds good, tried to keep the remaining internal call stack for unwinding.
Maybe we can keep the whole call stack by replacing consecutive external frames with an artificial frame introduced in the other patch. This should be synchronized with the LBR parser where we replace consecutive external LBRs with an artificial branch. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:525
+      // Do not ignore the entire samples, the remaining LBR can still be
+      // unwinded using a context-less stack.
+      continue;
----------------
nit: unwinded -> unwound


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:533
+        if (LBRStack.empty()) {
+          NumLeadingExternalLBR++;
+          // Record this LBR since current source and next LBR' target is still
----------------
nit: NumLeadingExternalLBR -> NumLeadingOutgoingLBR?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:217
+
+// The special frame address.
+enum SpecialFrameAddr {
----------------
nit: address -> addresses

Also enum class is preferred.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:305
+      return true;
+    return false;
+  }
----------------
nit: use a single return statement `return !LBRStack.empty() && LBRStack[0].Target == ExternalAddr` .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115538/new/

https://reviews.llvm.org/D115538