[PATCH] D124412: [BOLT] Report per-section hotness in bolt-heatmap.

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

That's a nice functionality, thanks!



================
Comment at: bolt/include/bolt/Profile/Heatmap.h:56-59
+    return (Address > MaxAddress) || (Address < MinAddress) ||
+           (!TextSections.empty() &&
+            (Address > TextSections.back().EndAddress ||
+             Address < TextSections.front().BeginAddress));
----------------
What happens if I want a heatmap that shows activity in addresses that are not mapped to a text section? (e.g. jitted code)

We need to support recording samples in unmapped regions as well.


================
Comment at: bolt/lib/Profile/DataAggregator.cpp:114
 
+static std::vector<SectionNameAndRange>
+getTextSections(const BinaryContext *BC) {
----------------
Instead of using static, wrap it in an anonymous namespace (just the extend the one we already have in lines 132-137).


================
Comment at: bolt/lib/Profile/Heatmap.cpp:256-259
+  if (TextSections.empty()) {
+    errs() << "No text sections captured from the binary.";
+    exit(1);
+  }
----------------



================
Comment at: bolt/lib/Profile/Heatmap.cpp:272
+  StringMap<uint64_t> SectionHotness;
+  int TextSectionIndex = 0;
+
----------------



================
Comment at: bolt/lib/Profile/Heatmap.cpp:277
+    auto Address = KV.first * BucketSize;
+    while (Address >= TextSections[TextSectionIndex].EndAddress)
+      TextSectionIndex++;
----------------
This and the next comment are optional. Either do that or assert(). The intent is to avoid crashing if Address is greater than the last EndAddress (I know this doesn't happen, but it is an invariant that would be nice if made explicit via an assertion).


================
Comment at: bolt/lib/Profile/Heatmap.cpp:279
+      TextSectionIndex++;
+    if (Address + BucketSize < TextSections[TextSectionIndex].BeginAddress) {
+      errs() << "Couldn't map the address bucket [0x"
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124412