[PATCH] D96434: [CSSPGO][llvm-profgen] Filter out the instructions without location info for symbolizer

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:60
       uint64_t PrevIP = IP.Address;
-      IP.backward();
+      HasNext = IP.backward();
       // Break into segments for implicit call/return due to inlining
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > Since all code addresses for the entire binary is record in a single vector, this `HasNext` check is only going to help for the first/last instruction of the entire binary. 
> > > > 
> > > > Is the problem you're seeing very specific to first/last instruction of the entire binary? I thought instruction without location could happen anywhere..
> > > Yeah, empty inline context should not be uncommon. Is the case where `IP.backward()` resulted in an invalid IP that crashed the `inlineContextEqual` check?
> > > this HasNext check is only going to help for the first/last instruction of the entire binary.
> > >IP.backward() resulted in an invalid IP that crashed the inlineContextEqual check
> > Exactly, you both are right.
> > 
> > > Is the problem you're seeing very specific to first/last instruction of the entire binary? I thought instruction without location could happen anywhere..
> > Yes, it could happen anywhere. if the instruction without location is not the first/ last instruction, it will jump to the next valid address which also get `false` for `inlineContextEqual`, it's fine for that. Only the first/last one, it will be out of boundary.
> > 
> > For our internal tool, it use a fake location, like <function_name, -1> which can ensure `inlineContextEqual` always return false. and I also saw the first/last instruction is always a fake location, so there is no out of boundary issue for it.
> > 
> > An alternative to fix this is to do the same as our internal one, for all the empty location stack, we create a fake location, say <invalid, -1>, then I guess we don't need to check the boundary for `advance` and `backward` (theoretically we should also check but we didn't see the case first/last insn isn't a fake location for our internal tool). One trade-off is it could sightly increase the memory usage. 
> > 
> > Any thoughts on this?
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> Thanks for the explanation. Can this out-of-range issue be fixed by making `inlineContextEqual` always return false for empty stacks? Tweaking `inlineContextEqual` so that no special care to out-of-bound IP sound cleaner to me.
Good point! I forgot it also needs to change `getInlineLeafFrameLoc` and `getExpandedContextStr` for the empty location stack check. 

Submitted the latest diff for the tweaking, ran them on the SPEC, it all passed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96434