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

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:154
+  auto maybeAddProbe = [&](DWARFDie Die) {
+    if (!isDIEOfProbe(Die))
+      return;
----------------
kyulee wrote:
> 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?
I'm not sure how I would move this since it needs a `DWARFDie` to read the variable name.


================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:161
+    uint64_t FunctionPtr =
+        dwarf::toAddress(Die.getParent().find(dwarf::DW_AT_low_pc))
+            .getValueOr(0);
----------------
kyulee wrote:
> IIRC, this die is attached to a function, so we try to get the function address from its parent?
The DIE we are looking at is assumed to be for a static variable (the probe). The parent DIE should be the function that owns the variable and that is the address we want. 


================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:182
+    }
+    if (!FunctionName || !CFGHash || !CounterPtr || !NumCounters) {
+      LLVM_DEBUG(llvm::dbgs()
----------------
kyulee wrote:
> 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.
`isDIEOfProbe()` only checks if the variable name starts with `__profc_`, so it's not unlikely that we run into this case. We could add the artificial flag or other attributes that make it more likely that this is a probe, then we could change this to an assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114566