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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1632
+        } else
+          NewFunctionSamples.emplace_back(Callee, *FS);
       }
----------------
huangjd wrote:
> davidxl wrote:
> > is it enough to just use a flag to indicate insertion happens (instead of using a vector)? The getOrCreateSamples can set the flag.
> The vector also stores a deep copy of the caller's FunctionSamples that's to be merged with newly created outlined function's FunctionSamples. Note that NonInlinedCallSites uses pointers to existing FunctionSamples, which will be invalidated if relocation happens, so we need a copy instead. 
Ok. Can you modify the comments to reflect this:  avoid creating new function samples (for outline function) in the the loop, otherwise the pointers used in the loop iteration can be invalidated.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1648
+  // existing FunctionSamples, so we need to update any fields with them.
+  if (NewFunctionSamples.size() > 0) {
+    updateFunctionSamplesPointers(F);
----------------
huangjd wrote:
> davidxl wrote:
> > insertion does not mean rehashing happens. Is there a better way to avoid the unncessary update?
> C++ unordered_map or llvm::DenseMap does not provide an interface to check if data are relocated. There's ugly way to check it such as comparing the address to the first element. However, rehashing is likely to happen if insertion happens, because the profile reader reserves the exact number of FunctionSamples in the map. Meanwhile insertion itself is unlikely to happen as this function is called very few times in a large production project, so it wouldn't matter for performance. 
make sense. Can you also update the comment saying this is rare event (i.e., missing function samples for outline function).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157061