[PATCH] D24989: Use profile info to set function section prefix to group hot/cold functions.

[Vedant Kumar via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:116
 
+void ProfileSummaryInfo::resetM(Module *newM) {
+  if (newM == M)
----------------
Please spell the method name out to be explicit.


================
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:120
+  M = newM;
+  delete Summary.release();
+}
----------------
Why aren't the Hot/Cold count thresholds cleared?


================
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:139
+  else
+    PSI->resetM(&M);
   return PSI.get();
----------------
How is the call to "resetM" being tested by your test?


================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:301
+  if (const Function *F = dyn_cast<Function>(GV)) {
+    const auto &Prefix = F->getSectionPrefix();
+    if (Prefix)
----------------
IMO this makes more sense as "auto OptionalPrefix =" or "Optional<StringRef> Prefix =".


================
Comment at: test/Transforms/CodeGenPrepare/section.ll:6
+
+; CHECK-OPT: hot_func{{.*}}!section_prefix ![[HOT_ID:[0-9]*]]
+; CHECK-OPT: cold_func{{.*}}!section_prefix ![[COLD_ID:[0-9]*]]
----------------
Please move your checks closer to the relevant "source" IR. Checks for module-level metadata should go at the end of the test, and checks related to a function should be near its definition.


================
Comment at: test/Transforms/CodeGenPrepare/section.ll:7
+; CHECK-OPT: hot_func{{.*}}!section_prefix ![[HOT_ID:[0-9]*]]
+; CHECK-OPT: cold_func{{.*}}!section_prefix ![[COLD_ID:[0-9]*]]
+; CHECK-OPT: ![[HOT_ID]] = !{!"function_section_prefix", !".hot"}
----------------
"[0-9]*" should be "[0-9]+" in these tests.


https://reviews.llvm.org/D24989