[PATCH] D100235: [CSSPGO][llvm-profgen] Fixing an obselete iterator issue.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D100235#2683311 <https://reviews.llvm.org/D100235#2683311>, @wenlei wrote:

> In D100235#2682347 <https://reviews.llvm.org/D100235#2682347>, @hoy wrote:
>
>> In D100235#2681312 <https://reviews.llvm.org/D100235#2681312>, @wenlei wrote:
>>
>>>> Another issue being fixed is that all outlined functions shared the same inline frame previously due to the unchanged Index value as the dummy inlineSite identifier.
>>>
>>> Why is this an issue? What problem does this cause? Conceptually, if we want to give them a tree structure, top level inliner are indeed children of dummy root.
>>
>> Yes, top level inliner is still children of dummy root with this change. However, they do not share dummy callsite id any more. Note that without this change, the dummy callsite id for top-level inliner (`Index` below) isn't updated correctly.
>>
>>   if (Root != Cur) {
>>      Index = readUnsignedNumber<uint32_t>();
>>    }
>
> Yeah, index would always be zero. But what problem does this cause? It seems that the call site id for top-level inliner is a dummy field anyways, so value should be ignored. Currently it works just fine, right? Is it also motivated by the upcoming change to report dangling probes with counts (so you need to different call site under dummy root)? If so, it might make sense to include it there as well..

The index starts from zero, and could be updated by parsing an inlinee. And when it comes back to the next top-level inliner, the value index is the leftover from the previous inlinee. This causes top-level inliner frames non-deterministically shared. The fix is motivated by upcoming dangling probe work but I feel that it may also affect inliner context building for shared top-level frames. I'll see if I can find a case for that. Otherwise I'll include this fix in the dangling probe patch as well.

>>>> They could be used to visit all probes that are associated to an inline frame, which is something else I'll be working on.
>>>
>>> We do have containers that can be re-allocated for growth, and doing that itself isn't an issue, unless it breaks specific use case. With what we have now we should not store pointers to inlinee's probe, and we're indeed doing that right now. So there's not bug, right?
>>>
>>> If there's a future change that require changing this model (of not keeping probe pointers), perhaps make the vector->list change together with the change that has the new use case?
>>
>> It doesn't break anything so far since `PseudoProbeInlineTree.ProbeVector` is not used anywhere. It might also be good to remove it for now since the container might end up with invalid pointers.
>>
>> Probe pointers isn't an issue as long as they are valid. I'm using `std::list` instead of `std::vector` to store real probe objects so that taking address of them is never invalidated.
>>
>> I'm actually working on a feature that reports all dangling probes in an inline frame into a profile, even if there are no samples collected for them. As of now only dangling probes with samples are reported. Since samples collected on a dangling probe doesn't make sense (dangling probes can be placed anywhere), they should always be reported so that the counts inferencer never overlooks them. It makes sense to include the current patch into that work.
>
> For dangling probe with counts, we currently use the special value (indicating dangling) instead of the actual count for output, right? What is the new behavior with upcoming change?

The new behavior lets you always report dangling probe regardless of whether there're samples collected for them. Dangling probes may be placed anywhere and if they are accidentally placed next to an instruction not executed, currently the dangling probe will not be reported. And when it comes to the counts inferencer, it won't know the probe is dangling. It'll treat the probe as never executed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100235