[PATCH] D34694: llvm-profdata: Indirect infrequently used fields to reduce memory usage

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/ProfileData/InstrProf.h:698
+  getValueSitesForKind(uint32_t ValueKind) {
+    auto AR = const_cast<const InstrProfRecord *>(this)->getValueSitesForKind(
+        ValueKind);
----------------
davidxl wrote:
> Is the const_cast here needed?  Non const pointer can be used with const method right?
Yep - otherwise this function would infinitely recurse - calling back into this non-const function.

Ideally we'd have an "implicit_cast<T>" function template that would make it more obvious this was only adding const, and not a dangerous thing like const_cast that can take away const.


================
Comment at: lib/ProfileData/InstrProf.cpp:511
+      getOrCreateValueSitesForKind(ValueKind);
   std::vector<InstrProfValueSiteRecord> &OtherSiteRecords =
+      Src.getOrCreateValueSitesForKind(ValueKind);
----------------
davidxl wrote:
> This one should use the read-only version of the API.
Ah, well, it does need to use the non-const, but not the 'creating' kind. 'merge' takes the InstrProfRecord by non-const ref to call sort on it, it seems, just in case it's not already sorted.


================
Comment at: lib/ProfileData/InstrProf.cpp:542
   std::vector<InstrProfValueSiteRecord> &ThisSiteRecords =
-      getValueSitesForKind(ValueKind);
+      getOrCreateValueSitesForKind(ValueKind);
   for (uint32_t I = 0; I < ThisNumValueSites; I++)
----------------
davidxl wrote:
> Should use the read-only version.
Right right - like the other one, it doesn't need the 'orCreate' part, but it does need the non-const (MutableArrayRef) to scale the elements.


https://reviews.llvm.org/D34694