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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> 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.

> 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?



================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.cpp:217
+    if (Root == Cur) {
+      // Use a sequential id for outline functions.
+      Index = Root->getChildren().size();
----------------
nit: outline function -> top level inliner.


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:74
 
+  auto &getChildren() { return Children; }
   void addProbes(PseudoProbe *Probe) { ProbeVector.push_back(Probe); }
----------------
Can we spell out the full return type? It seems we would use typedef if needed, but not so for `auto` return type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100235