[PATCH] D114566: [InstrProf] Add Correlator class to read debug info

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee added inline comments.


================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:90
+        ShouldSwapBytes);
+  }
+  return make_error<InstrProfError>(
----------------
It's nor blocking but it might be a more relevant error message when it fails for COFF -- not yet implemented or supported case.


================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:154
+  auto maybeAddProbe = [&](DWARFDie Die) {
+    if (!isDIEOfProbe(Die))
+      return;
----------------
It's concise to have it here but should we check this outside so that we don't need to come here after creating unnecessary DWARFDie?


================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:161
+    uint64_t FunctionPtr =
+        dwarf::toAddress(Die.getParent().find(dwarf::DW_AT_low_pc))
+            .getValueOr(0);
----------------
IIRC, this die is attached to a function, so we try to get the function address from its parent?


================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:182
+    }
+    if (!FunctionName || !CFGHash || !CounterPtr || !NumCounters) {
+      LLVM_DEBUG(llvm::dbgs()
----------------
My understanding is that FunctionName, CFGHash, and NumCounters are attached to this correlated die always under this flag.
Should we assert them instead? If they are null, I wonder what situation we might run into.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114566