[PATCH] D56435: We can improve the performance (generally) by memo-izing the action to map a debug location to its function summary.

[David Callahan via Phabricator via llvm-commits]

david2050 marked 3 inline comments as done.
david2050 added inline comments.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:221
   findIndirectCallFunctionSamples(const Instruction &I, uint64_t &Sum) const;
+  mutable DenseMap<const DILocation *, const FunctionSamples *> DILocation2SampleMap;
   const FunctionSamples *findFunctionSamples(const Instruction &I) const;
----------------
Any guidance on the use of mutable v. removing the "const" attributes on the 3 affected method? 


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:728-732
+  auto it = DILocation2SampleMap.find(DIL);
+  if (it != DILocation2SampleMap.end())
+    return it->second;
+  const FunctionSamples *s = Samples->findFunctionSamples(DIL);
+  DILocation2SampleMap.try_emplace(DIL, s);
----------------
dblaikie wrote:
> Perhaps do an unconditional try_emplace and use the bool in the returned pair to decide if the work has already been done or not? This saves two map lookups on the first lookup.
> 
>   pair<iterator, bool> p = try_emplace(DIL, nullptr);
>   if (p.second)
>     p.first->second = findFunctionSamples(DIL)
>   return p.first->second;

I was uncertain if the iterator was save over the call but will update and in parallel test the change locally.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56435