[PATCH] D114566: [InstrProf] Add Correlator class to read debug info
Ellis Hoag via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 30 17:07:27 PST 2021
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
More information about the llvm-commits
mailing list