[PATCH] D157061: [SampleProfile] Potential use after move in SampleProfileLoader::promoteMergeNotInlinedContextSamples

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1650-1652
+  if (NewFunctionSamples.size() > 0) {
+    updateFunctionSamplesPointers(F);
+  }
----------------
Patching all pointers seems a bit hacky. Have you considered maintaining reference stability by using a separate container for new function samples, so there will be no insertion to SampleProfileReader::Profiles? 

Strictly speaking SampleProfileReader::Profiles should only contain profile directly read out of input, and those newly created profiles are synthesized which doesn't belong to reader. 

If we use a separate container (say `SynthesizedFuncSamples`), we would only need to change the code below to look for profiles from the addition container as well for non-CS profile. 

```
  if (FunctionSamples::ProfileIsCS)
    Samples = ContextTracker->getBaseSamplesFor(F);
  else
    Samples = Reader->getSamplesFor(F);
```

--> 

```
  if (FunctionSamples::ProfileIsCS)
    Samples = ContextTracker->getBaseSamplesFor(F);
  else {
    Samples = Reader->getSamplesFor(F);
    if (!Samples)
      Samples = findSynthesizedProfile(F);
  }
```


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1658-1659
+void SampleProfileLoader::updateFunctionSamplesPointers(const Function &F) {
+  assert(Reader->profileIsCS() == FunctionSamples::ProfileIsCS);
+  if (Reader->profileIsCS())
+    ContextTracker = std::make_unique<SampleContextTracker>(
----------------
Yeah we should assert this is non CS profile. CS profile doesn't need promoteMergeNotInlinedContextSamples as all adjustments are done in SampleContextTracker without creating new FunctionSamples.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1660-1661
+  if (Reader->profileIsCS())
+    ContextTracker = std::make_unique<SampleContextTracker>(
+        Reader->getProfiles(), &GUIDToFuncNameMap);
+  if (FunctionSamples::ProfileIsCS)
----------------
This is super heavy hammer to rebuild all context trie, which would have been a deal breaker. But again, we shouldn't need any of this for CS profile. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157061